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.

Sign up for GitHub

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

Jump to bottom

Fermi 2nd and 3rd Pulsar Catalog #4699

Closed
wants to merge 11 commits into from
Closed

Fermi 2nd and 3rd Pulsar Catalog #4699

wants to merge 11 commits into from

Conversation

MRegeard
Copy link
Member

This pull request introduce the 2PC Fermi-LAT catalog to gammapy.catalog sub-package.

The catalog fits file consist of 2 main HDUs:

  • One is for the actual pulsar catalog. It gathers intrinsic information on the pulsars, such as the position, the period, derivative of the period, etc.
  • The other one is for the spectral parameters of the pulsars.

Therefore, I had to modify a little bit the core class SourceCatalog and SourceCatalogObject to account for the spectral parameters to be in another HDU (as with the extended_source_table).

I still need to make the SourceCatalogObject class for this catalog.

@MRegeard MRegeard marked this pull request as draft July 26, 2023 14:42
@bkhelifi bkhelifi requested a review from QRemy July 27, 2023 12:12
@bkhelifi bkhelifi added this to the 1.2 milestone Jul 27, 2023
bkhelifi
bkhelifi reviewed Jul 27, 2023
View reviewed changes
gammapy/catalog/core.py Outdated
return source

@lazyproperty
def _lookup_spectral_source_idx(self):
names = [_.strip() for _ in self.spectral_table["PSR_Name"]]
Copy link
Member

Choose a reason for hiding this comment

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

Is it not a bit surprising that in the `SourceCatalog' class the spectral parameters are only for pulsar? Is this function well placed?

Copy link
Contributor

@QRemy QRemy Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

"PSR_Name" could be replaced by self._source_name_key and same for "Source_Name" in _lookup_extended_source_idx

Copy link
Member Author

Choose a reason for hiding this comment

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

@bkhelifi, in other catalogs the main hdu contains the spectral parameters, while in the pulsar catalog the main hdu contain the pulsar specs.

@bkhelifi
Copy link
Member

@MRegeard , is it ready to be reviewed or some work is still needed to add later on the 3PC?

@MRegeard
Copy link
Member Author

@MRegeard , is it ready to be reviewed or some work is still needed to add later on the 3PC?

@bkhelifi, No some work is still needed here...

@codecov
Copy link

codecov bot commented Oct 2, 2023 •
edited
Loading

Codecov Report

Merging #4699 (b3d5973) into main (3378185) will decrease coverage by 0.52%.
Report is 99 commits behind head on main.
The diff coverage is 18.92%.

@@            Coverage Diff             @@
##             main    #4699      +/-   ##
==========================================
- Coverage   75.53%   75.01%   -0.52%     
==========================================
  Files         223      226       +3     
  Lines       32666    33396     +730     
==========================================
+ Hits        24675    25053     +378     
- Misses       7991     8343     +352
Files Coverage Δ
gammapy/catalog/__init__.py 100.00% <ø> (ø)
gammapy/catalog/core.py 70.87% <64.28%> (-1.74%) ⬇️
gammapy/catalog/fermi.py 14.77% <15.03%> (+0.11%) ⬆️

... and 18 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

bkhelifi
bkhelifi reviewed Oct 3, 2023
View reviewed changes
gammapy/catalog/fermi.py
)

@property
def flux_points(self):
Copy link
Member

Choose a reason for hiding this comment

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

I would put in argument the format

bkhelifi
bkhelifi reviewed Oct 3, 2023
View reviewed changes
gammapy/catalog/fermi.py Outdated Show resolved Hide resolved
bkhelifi
bkhelifi reviewed Oct 3, 2023
View reviewed changes
gammapy/catalog/fermi.py

One source is represented by `~gammapy.catalog.SourceCatalogObject2PC`.

TODO : Fix the UnitsWarning here ...
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Even with the warning filter when reading the table, I still have a units warning, which is really annoying.

@MRegeard MRegeard marked this pull request as ready for review October 19, 2023 10:41
@MRegeard
2PC catalog Source class and modif fermi core
c0fbf0a 
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard
replace source name by key
63bbcb0 
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard
base class first method
9ce8ecc 
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard
adding string + flux points + spectral model
fa95e46 
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard
adding the 3PC
3f37832 
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard
fix spectral points
a447554 
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard
adding tests
cb41eb6 
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard
fix test str
794f573 
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard
remove TODO
4d47712 
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard
Copy link
Member Author

I think that I made most of the implementations here. We still miss the light curve implementation, that is pending issue #4782.

@MRegeard MRegeard changed the title Fermi 2nd Pulsar Catalog Fermi 2nd and 2rd Pulsar Catalog Oct 19, 2023
@MRegeard MRegeard changed the title Fermi 2nd and 2rd Pulsar Catalog Fermi 2nd and 3rd Pulsar Catalog Oct 19, 2023
Astro-Kirsty
Astro-Kirsty reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@Astro-Kirsty Astro-Kirsty 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 inline question and comment!

gammapy/catalog/core.py
if self._source_name_key in data and self._source_name_key == "PSR_Name":
name_spectral = data[self._source_name_key].strip()
elif self._source_name_key in data and self._source_name_key == "PSRJ":
name_spectral = f"PSR{data[self._source_name_key].strip()}"
Copy link
Member

Choose a reason for hiding this comment

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

Is the 'J' for 'PSRJ' already connected to data[self._source_name_key] ?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean ?

gammapy/catalog/fermi.py Outdated Show resolved Hide resolved
@MRegeard
changing fluxe to flux_err
0d1996b 
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard
change filename in test
b3d5973 
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard MRegeard modified the milestones: 1.2, 1.3 Jan 24, 2024
@MRegeard
Copy link
Member Author

I will split this big PR into small pieces.

This was referenced Jan 30, 2024
Merged
Merged
Merged
@QRemy
Copy link
Contributor

QRemy commented May 3, 2024

Splitted into 3 PRs so closing this one.

@QRemy QRemy closed this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkhelifi bkhelifi bkhelifi left review comments

@QRemy QRemy QRemy left review comments

@Astro-Kirsty Astro-Kirsty Astro-Kirsty left review comments

Assignees
No one assigned
Labels
feature
Projects
gammapy.catalog
  
Awaiting triage
Milestone
1.3
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@MRegeard @bkhelifi @QRemy @Astro-Kirsty @registerrier