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

Fix GK data saving. #525

Merged
merged 12 commits into from
Jul 3, 2022
Merged

Fix GK data saving. #525

merged 12 commits into from
Jul 3, 2022

Conversation

SamTov
Copy link
Member

@SamTov SamTov commented Jun 3, 2022

Fix #524

In this PR I correct how data is added to the SQL database in the GK calculators. Previously, the final integration value was stored whilst the integration range value was printed. This is corrected and the enhanced visualization added to the GK calculators.

Added tests:

None yet but before converted to real PR there will be some tests to ensure the data is saved correctly.

TODO

  • Test IC
  • Add tests in integration test suite.

@SamTov SamTov requested a review from christophlohrmann June 14, 2022 06:31
@SamTov SamTov changed the title [WIP ] Fix GK data saving. Fix GK data saving. Jun 29, 2022
@SamTov
Copy link
Member Author

SamTov commented Jun 29, 2022

@christophlohrmann it is at the state we discussed today.

@SamTov
Copy link
Member Author

SamTov commented Jul 1, 2022

Had the pubchempy server request failure.

@SamTov SamTov requested a review from PythonFZ July 1, 2022 06:50
Copy link
Member

@PythonFZ PythonFZ left a comment

Choose a reason for hiding this comment

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

Just a few smaller comments.

For the notebook it currently shows Could not load dynamic library 'libcudnn.so.8'. Should we run it on a machine with cuda installed so that these errors don't occur?

  • It should be data["NaCl"]["Na"]["diffusion_coefficient"] not data["NaCl"].data_dict["Na"]["diffusion_coefficient"]
  • maybe print KCl_data = KCl_file.get_file('.')[0] so that a user not using datahub knows what to pass the simulation_data

@SamTov
Copy link
Member Author

SamTov commented Jul 1, 2022

I think the data dict stuff should be changed in all calculators and tests and areas so perhaps we also do this in a seperate PR that focuses on it.

@SamTov SamTov merged commit 9cae83a into main Jul 3, 2022
@SamTov SamTov deleted the SamTov_IC_Patch branch July 3, 2022 13:00
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.

Integration range parameter is not what get's stored in the SQL database
3 participants