Skip to content

ENH: adding roman simulation density tutorial #93

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

Merged

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Apr 8, 2025

I add this notebook to the pages now, but I would like to have the thumbs up from @jkrick and @vandesai1 about e.g the placing, etc.

Outstanding todos I noticed so far, but we should add to the list as they come up:

  • rename download directory to data to have a bit more agreement amongst tutorials (this maybe a more overarching issue as the others may not be consistent yet either)
  • We may want to say a bit more about the dataset, and link to the IRSA website of it, etc.

@bsipocz bsipocz added the enhancement New feature or request label Apr 8, 2025
@bsipocz bsipocz requested review from jkrick and vandesai1 April 8, 2025 21:53
Copy link
Contributor

@jkrick jkrick left a comment

Choose a reason for hiding this comment

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

I was thinking this notebook would fit under "Accessing IRSA’s on-premises holdings using VO protocols"

@bsipocz
Copy link
Member Author

bsipocz commented Apr 17, 2025

@jkrick - but it doesn't use VO protocols but the data dump in h5py files.

@bsipocz
Copy link
Member Author

bsipocz commented Apr 17, 2025

@jkrick - Hmm, I've just looked at the rendered plots and the last jackknife one is flat while the caption claims otherwise. Is that expected? I don't see any errors hit by the code cells.

@jkrick
Copy link
Contributor

jkrick commented Apr 17, 2025

good catch! That plot will only look interesting if all the files are used, but we have it setup to just download and use 1 file. I can change the caption to reflect this.

I also see that I still need to move the functions out to a code_src type directory. Or would you rather leave it all as one file?

Let me know the order in which I should do these things, ie., wait for them to be on iras-tutorials and then write a PR or do a PR against the current ipac-sp-notebooks?

@bsipocz
Copy link
Member Author

bsipocz commented Apr 17, 2025

I also see that I still need to move the functions out to a code_src type directory. Or would you rather leave it all as one file?

We don't do code_src here, all the notebooks are expected to be self contained without outside code, or data, or requirements files.

Let me know the order in which I should do these things, ie., wait for them to be on iras-tutorials and then write a PR or do a PR against the current ipac-sp-notebooks?

A PR here would be preferable.

@jkrick
Copy link
Contributor

jkrick commented Apr 17, 2025

ok, added issue #97 to remind me to change the figure caption once the notebook is merged here.

@bsipocz bsipocz merged commit 4733958 into Caltech-IPAC:main Apr 17, 2025
8 checks passed
@bsipocz bsipocz deleted the ENH_adding_roman_simulation_density branch April 17, 2025 21:52
github-actions bot pushed a commit that referenced this pull request Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants