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 get scan path #284

Merged
merged 3 commits into from
Jul 13, 2023
Merged

Add get scan path #284

merged 3 commits into from
Jul 13, 2023

Conversation

ercius
Copy link
Collaborator

@ercius ercius commented May 9, 2023

This adds a function to return the path to a counted data set using the scan_id, scan_num, and/or threshold to io.

@cjh1
Copy link
Member

cjh1 commented May 9, 2023

@ercius I can see the utility of this function, however, I wonder if it might be better off in another location that is more site specific. I worry about encoding distiller conventions in stempy. Maybe included inline in the notebooks? Another alternative would be to put it in a contrib directory, indicating that its not part of the stempy public interface. I am open to other options too.

@ercius
Copy link
Collaborator Author

ercius commented May 10, 2023

I wonder if it might be better off in another location that is more site specific. I worry about encoding distiller conventions in stempy.

That makes sense. Im happy to move it anywhere that makes sense.

Maybe included inline in the notebooks?

I was trying to avoid this because it adds unnecessary code and looks really cluttered. Maybe that is just something to live with? It is annoying to copy this between notebooks and makes it more prone to getting out of sync across notebooks.

Another alternative would be to put it in a contrib directory, indicating that its not part of the stempy public interface.

I think that would work too. It would be a simple import in the notebooks that need it. This seems like the best trade-off to me.

@ercius
Copy link
Collaborator Author

ercius commented May 19, 2023

After our recent discussion I think contrib is a great alternative. Ill update the PR.

@ercius
Copy link
Collaborator Author

ercius commented May 23, 2023

@cjh1 This works in my development environment. Let me know what you think about merging it

@cjh1 cjh1 merged commit 0bb5eaa into OpenChemistry:master Jul 13, 2023
@ercius ercius deleted the add_get_scan_path branch March 27, 2024 15:56
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