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

Refactor notebook #90

Merged
merged 12 commits into from
Sep 21, 2023
Merged

Refactor notebook #90

merged 12 commits into from
Sep 21, 2023

Conversation

awf
Copy link
Collaborator

@awf awf commented Sep 18, 2023

The existing DFT notebook simply calls the density_functional_theory.py version using os.system. This change moves more of the python logic into the notebook, moving density_functional_theory.py to pyscf_ipu/dft.py.

@awf awf force-pushed the awf/refactor-notebook branch from 43ae559 to 1e5e1e4 Compare September 18, 2023 20:05
@awf awf marked this pull request as ready for review September 18, 2023 20:33
@@ -7,31 +7,23 @@
"source": [
Copy link
Collaborator Author

@awf awf Sep 20, 2023

Choose a reason for hiding this comment

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

Line #6.    loaded = os.path.exists(out_filename) and os.path.getsize(out_filename) == 6985727

HH: Use a proper hash?


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea - will add to the backlog.

Copy link
Contributor

@hatemhelal hatemhelal left a comment

Choose a reason for hiding this comment

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

good to go after clearing the outputs 🚢

@AlexanderMath
Copy link
Contributor

AlexanderMath commented Sep 21, 2023

Notes (second code block)

Code fails at tar -xvf ./data/gdb11.tgz --directory ./gdb/. The folder gdb/ doesn't exist and tar is not allowed to create (running locally on 3b76ef5). The line below tries to load from /data/ , that is, sortgdb.sort_gdb looks at gdb_filename="./data/gdb11_size09.smi". Changing to tar -xvf ./data/gdb11.tgz --directory ./data/ seem to fix the issue. (when debugging using -nc with wget circumvents excessive re-downloading)

Let me know if for some reason I'm at the wrong commit/branch (local code looks identical to cell block in ReviewNB)

@AlexanderMath
Copy link
Contributor

AlexanderMath commented Sep 21, 2023

Managed to reproduce the jax.jit tracing causing time to increase from ~100ms to 5000ms.

@AlexanderMath
Copy link
Contributor

AlexanderMath commented Sep 21, 2023

Issue is resolved for me if I don't re-create dcargs for every conformer. @awf

if conformer_num == 0: dcargs = namedtuple('dcargs', dcargs_names)(*(args.__dict__[a] for a in dcargs_names))

@awf
Copy link
Collaborator Author

awf commented Sep 21, 2023

Notes (second code block)

Code fails at tar -xvf ./data/gdb11.tgz --directory ./gdb/. The folder gdb/ doesn't exist ...

Let me know if for some reason I'm at the wrong commit/branch (local code looks identical to cell block in ReviewNB)

Fixed, thanks.

@awf
Copy link
Collaborator Author

awf commented Sep 21, 2023

Issue is resolved for me if I don't re-create dcargs for every conformer. @awf

if conformer_num == 0: dcargs = namedtuple('dcargs', dcargs_names)(*(args.__dict__[a] for a in dcargs_names))

Thanks for the scoping, which led me to understand that it's not the re-creation of dcargs, but of its type, that is namedtuple('dcargs', dcargs_names)

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@awf awf merged commit a3208e2 into main Sep 21, 2023
4 checks passed
@awf awf deleted the awf/refactor-notebook branch September 21, 2023 17:32
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.

3 participants