Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added new API support for nextgen Materials Project #692

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

sundusaijaz
Copy link
Collaborator

No description provided.

ken-sc01 and others added 7 commits February 6, 2025 14:03
* update tensorboard logdir in tutorials 2 and 3 and formatting in 5

* minor tutorial fixes
* [bug/qm7x_groupsplit] Adding GroupSplit to the exported names for splitting.py bc it was missing and causing import errors in qm7x dataset

* [bug/qm7x_groupsplit] Fixing black fmt error
* update tensorboard logdir in tutorials 2 and 3 and formatting in 5

* minor tutorial fixes

* import GroupSplit for QM7X

* add progressbar dependency

* add progressbar dependency

* add qm7x data config file
Copy link
Collaborator

@stefaanhessmann stefaanhessmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I added some minor remarks

builder.docx Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is changed in this notebook? Is it on purpose?

Copy link
Collaborator Author

@sundusaijaz sundusaijaz Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, It was not mine

@@ -103,12 +105,20 @@ def __init__(
distance_unit=distance_unit,
**kwargs,
)
if len(apikey) != 16:
"""if len(apikey) != 16:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be removed

@@ -120,6 +130,8 @@ def prepare_data(self):
MaterialsProject.EPerAtom: "eV",
MaterialsProject.BandGap: "eV",
MaterialsProject.TotalMagnetization: "None",
MaterialsProject.MaterialId: "None",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this to property unit dict is not needed, because we store the metadata in the key_value_pairs and not in data. So these 2 lines can be removed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initiallly, when I was getting some key error on material_id which resolved by doing this. let me test again after removing this. Then I will update

MaterialsProject.BandGap: q.band_gap,
}
)
# todo: use key-value-pairs or not?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment

)

for q in query:
# if (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment

)

with MPRester(self.apikey) as m:
# for N in range(1, 9):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants