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

Add sim and path config yaml files #16

Merged
merged 3 commits into from
Nov 18, 2021
Merged

Add sim and path config yaml files #16

merged 3 commits into from
Nov 18, 2021

Conversation

jedyeo
Copy link
Collaborator

@jedyeo jedyeo commented Nov 15, 2021

Add sim and path config yaml files for the simulator to use.

Code to receive and parse these yamls will be added in a future PR (soon).

@calhep @thisTyler @arjunsingh3600

@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #16 (a425348) into master (6ee4446) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #16   +/-   ##
=======================================
  Coverage   96.62%   96.62%           
=======================================
  Files           4        4           
  Lines         118      118           
=======================================
  Hits          114      114           
  Misses          4        4           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ee4446...a425348. Read the comment docs.

@@ -0,0 +1,37 @@
molecular_model:
voxel_size: 0.1 # [nm]
Copy link
Contributor

Choose a reason for hiding this comment

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

are these string paths ever read by the TEM simulator? Or do we have freedom to change their names?
e.g. voxel_size to pixel_size_ang. If these are the "expected fields" to the TEM simulator, then I'm thinking we can have another layer above where standard params that are shared between different simulation routines are converted into "tem simulator" specific ones (units, etc).

@@ -0,0 +1,5 @@
pdb_dir: '../temp_workspace/input/'
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we were going to get rid of a working directory... if it's something that only exists for the length of the simulation, perhaps it could be created and deleted with the os module

Copy link
Collaborator

Choose a reason for hiding this comment

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

This particular working directory exists only for demonstration purposes - this path would be changed out by the user. temp_workspace/ should not be in the PR - I'll look into addressing this after work today.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

  • having the pdb data in a data/ or pdb_structures/ directory inside simSPI/simSPI
  • having the config files at the root of the simSPI/simSPI repo
    would be more intuitive?

We do not need to tell the users how to organize their workspace.

Copy link
Contributor

Choose a reason for hiding this comment

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

And the user can call the simulator by providing a path output_dir to tell it where to store the output of the simulation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good ideas, thank you!

I've edited the folder structure - temp_workspace is gone (as I mentioned earlier it's best not left in the main branch anyhow) and the config files now sit in the "root" simSPI/ directory. The PDBs are stored in simSPI/data as you suggested.

For now, I think simSPI/output/ makes sense as a default output directory and the user can change it in the config file if desired.

Copy link
Contributor

@ninamiolane ninamiolane left a comment

Choose a reason for hiding this comment

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

Looking good, thanks for doing the effort of splitting the work into block! 🙌 See my comment about the temp_workspace.

@ninamiolane ninamiolane merged commit c5703dc into master Nov 18, 2021
@fredericpoitevin fredericpoitevin deleted the add-yaml-files branch April 13, 2022 15:55
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