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

Surface Diffusion Estimator Example and Associated Improvements #13

Merged
merged 10 commits into from
Mar 21, 2024

Conversation

mjohnson541
Copy link
Collaborator

This primarily adds an example notebook for estimating surface diffusion coefficients with single evaluation SIDT. This primarily required improvements to how we save and load tree nodes to handle non-JSON serializable objects.

@mjohnson541 mjohnson541 requested a review from hwpang March 20, 2024 01:32
Copy link
Collaborator

@hwpang hwpang left a comment

Choose a reason for hiding this comment

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

Can you also format the notebook? Mostly to add blank after commas. You can pip install "black[jupyter]" and run that through the notebook.

pysidt/sidt.py Outdated Show resolved Hide resolved
pysidt/sidt.py Outdated Show resolved Hide resolved
except (TypeError, OverflowError):
rule = to_dict(node.rule) #will work on all rmgmolecule objects, new objects need this method implemented
try:
json.dumps(rule)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this right? Do you not need to open a file handler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This generates the serialization without putting it in a file.

pysidt/sidt.py Outdated
try:
json.dumps(rule)
except:
print(rule)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you write proper logger and informative error message

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've provided a more informative error message and removed the logging.

pysidt/sidt.py Outdated Show resolved Hide resolved
pysidt/sidt.py Outdated Show resolved Hide resolved
pysidt/sidt.py Outdated Show resolved Hide resolved
pysidt/sidt.py Outdated Show resolved Hide resolved
@mjohnson541 mjohnson541 force-pushed the diffusion_estimator_improvements branch from 3d04877 to 6e546df Compare March 21, 2024 00:18
@mjohnson541
Copy link
Collaborator Author

I formatted the notebook manually. Should be ready.

Copy link
Collaborator

@hwpang hwpang left a comment

Choose a reason for hiding this comment

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

More questions regarding the code

- cairo
- cairocffi
- pyrdl
- python >=3.7,<3.10
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 blocking 3.10?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have rmgmolecule for >3.9

pysidt/sidt.py Outdated
out_dict[attr] = val
except:
if isinstance(val, ScalarQuantity):
d = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this right? d is not used anywhere? Can we add test for this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, we do test this in the notebook, although it's more of a functional test through write_nodes. I can construct a nosetests framework. I'm not very familiar with pytest though, so if we want to continue using pytest it's probably most efficient if you setup the framework.

pysidt/sidt.py Outdated Show resolved Hide resolved
pysidt/sidt.py Show resolved Hide resolved
@mjohnson541 mjohnson541 force-pushed the diffusion_estimator_improvements branch from e45d2ea to 84d4bdc Compare March 21, 2024 19:55
@mjohnson541 mjohnson541 force-pushed the diffusion_estimator_improvements branch from 84d4bdc to 1b71b52 Compare March 21, 2024 20:05
@mjohnson541
Copy link
Collaborator Author

I'm a bit unsure about some of the choices the formatter made in the notebook, I think it was better before.

@mjohnson541 mjohnson541 force-pushed the diffusion_estimator_improvements branch from 1b71b52 to 40dad62 Compare March 21, 2024 20:17
Copy link
Collaborator

@hwpang hwpang left a comment

Choose a reason for hiding this comment

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

LGTM

@hwpang hwpang merged commit 8102c8a into main Mar 21, 2024
1 check passed
@hwpang hwpang deleted the diffusion_estimator_improvements branch March 21, 2024 21:13
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.

2 participants