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 cells to download files in grb & crab spectral fit notebooks #144

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

eneights
Copy link
Contributor

Add cells in the grb & crab spectral fitting notebooks to download the necessary files from wasabi

@eneights eneights requested a review from ckarwin February 23, 2024 17:08
@ckarwin
Copy link
Contributor

ckarwin commented Feb 26, 2024

Hi @eneights, Thanks for making these changes. I reviewed your PR, and all the new cells are working. There are still some small details that I think need to be updated though, specified below. Also, it looks like the line fit example has not been updated yet. Were you planning to do this as well?

Both Crab and GRB notebooks:

  1. To run this, you need the following files, which can be found on wasabi and downloaded using the first few cells of this notebook:

Most users won't have access to wasabi, and so I think you can just say that the files need to be downloaded using the first few cells of the notebook.

  1. You still have your personal data path in the first cell of the crab notebook. I think the default can just be ".", which is the working directory. Or "path/to/files", as you have in the GRB notebook.

  2. Can you please also add the file sizes for each cell, in order to give users a sense of how long the download will take? They can be found on wasabi.

  3. I think it would be sufficient to add a comment before the cells specifying that the files should only be downloaded if not previously downloaded, instead of repeating this for each cell.

@eneights
Copy link
Contributor Author

Thanks @ckarwin! I made those changes.

The point source line fitting notebook should probably be deleted, do you want me to do that? It's for the Compton sphere and the extended source 511 notebook replaces it.

@ckarwin
Copy link
Contributor

ckarwin commented Feb 26, 2024

Thanks for making the changes, @eneights.

Ok, I see. I think it's probably ok to remove the line fitting notebook. Let's double check with @israelmcmc, do you see any problems with this?

@israelmcmc
Copy link
Collaborator

I also agree it's good to remove it. Conceptually, the jump from a PL or band function to a Dirac delta (or a narrow Gaussian) is not that big, so I think the users will not get lost if the line emission analysis is introduced together with the extended source emission ---which is the realistic use case anyways. The 511-emitting "GRB" had its purpose when were trying to figure out the implementation and it allowed us to tackle one problem at a time, but at this point, it's better to reduce the number of notebooks that we need to maintain.

@ckarwin
Copy link
Contributor

ckarwin commented Feb 26, 2024

Ok, @eneights, please go ahead and remove the line fit NB, and then I can merge this PR.

In the second line of the extended source NB it says:

For a general introduction into spectral fitting with cosipy, see the continuum_fit and line_fit tutorials.

Can you also delete:

and line_fit tutorials

@eneights
Copy link
Contributor Author

@ckarwin I deleted the line fit tutorial and its references in the extended source notebook and other_examples.rst.

Copy link
Contributor

@ckarwin ckarwin left a comment

Choose a reason for hiding this comment

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

Thanks, @eneights.

@ckarwin ckarwin merged commit bfaa42f into cositools:main Feb 28, 2024
1 check passed
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