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

Update pulsar recipe #22

Merged
merged 9 commits into from
Aug 30, 2024
Merged

Conversation

MRegeard
Copy link
Member

This PR update the pulsar recipe to Gammapy version 1.2 and PINT version 1.0. I also corrected the issue with phase not in the event table print at the end of the recipe.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Member

@AtreyeeS AtreyeeS left a comment

Choose a reason for hiding this comment

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

Thanks a lot @MRegeard ! This is useful
Since the diff is not very readable, I leave comments here

  1. Please clear the outputs
  2. in 0. Dependencies and Imports, update gammapy-1.0-environment.yml to 1.2
  3. In the target_position = SkyCoord()... do we need the ra and dec to this precision?
  4. In Section 2.1, where you mention checking the START and FINISH entries, maybe you can show how to do that, and that it is valid for this run (using the GTI for the run)
  5. The point with deleting the JUMP parameter has to be explained better. Open the file, comment it out, save it, etc. Isn't there any better way?
  6. remove the blank cell at the end

@MRegeard
Copy link
Member Author

Thanks @AtreyeeS for you comments.

  1. Done
  2. Done
  3. Done
  4. I described a little bit better this part and show how one can check for time using tstart and tstop of the observation.
  5. I also explain a little bit better this part. Unfortunately, we cannot really do anything with this, it cause an error on PINT side. I also try to use the Crab ephemeris provided by Fermi 3PC, but it is even worse because it is written as TCB, which PINT does not support. One would have to use Tempo2 to convert it to TDB.
  6. Done.

recipes/pulsar_phase/pulsar_phase_computation.ipynb Outdated Show resolved Hide resolved
recipes/pulsar_phase/pulsar_phase_computation.ipynb Outdated Show resolved Hide resolved
recipes/pulsar_phase/pulsar_phase_computation.ipynb Outdated Show resolved Hide resolved
recipes/pulsar_phase/pulsar_phase_computation.ipynb Outdated Show resolved Hide resolved
recipes/pulsar_phase/pulsar_phase_computation.ipynb Outdated Show resolved Hide resolved
recipes/pulsar_phase/pulsar_phase_computation.ipynb Outdated Show resolved Hide resolved
recipes/pulsar_phase/pulsar_phase_computation.ipynb Outdated Show resolved Hide resolved
@Astro-Kirsty
Copy link
Member

Thanks @MRegeard. I've had a look and made some comments. I will also send you some comments via slack which were I was not able to make review comments.
Thanks again!

Co-authored-by: Astro-Kirsty <AstroKirsty@gmail.com>
@MRegeard
Copy link
Member Author

@Astro-Kirsty, Thanks you very much for your review !

Copy link
Member

@AtreyeeS AtreyeeS left a comment

Choose a reason for hiding this comment

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

Thank you @MRegeard ! No further comments. If the comments by @Astro-Kirsty have been addressed, we can merge

recipes/pulsar_phase/pulsar_phase_computation.ipynb Outdated Show resolved Hide resolved
@MRegeard
Copy link
Member Author

We can merge this PR, what do you think @AtreyeeS @registerrier ?

Copy link
Contributor

@registerrier registerrier left a comment

Choose a reason for hiding this comment

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

Thanks @MRegeard . See few inline comments.

"\n",
"\n",
"`$ conda env create -n gammapy-pint -f gammapy-1.0-environment.yml`\n",
"`$ conda env create -n gammapy-pint -f gammapy-pint-environment.yml`\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this gammapy-pint-environment.yml ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really know. I think that is was the original env file that was presented to the user. But then we also need this env.yml for the CI. In the end they are the same so I could make gammapy-pint-environment.yml a simlink to env.

recipes/pulsar_phase/pulsar_phase_computation.ipynb Outdated Show resolved Hide resolved
recipes/pulsar_phase/pulsar_phase_computation.ipynb Outdated Show resolved Hide resolved
recipes/pulsar_phase/pulsar_phase_computation.ipynb Outdated Show resolved Hide resolved
MRegeard and others added 4 commits July 12, 2024 20:26
Co-authored-by: Régis Terrier <regis.terrier@m4x.org>
Co-authored-by: Régis Terrier <regis.terrier@m4x.org>
Co-authored-by: Régis Terrier <regis.terrier@m4x.org>
@registerrier registerrier merged commit 9c0586b into gammapy:master Aug 30, 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.

4 participants