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

Fermi pulsar catalogs base class #5056

Merged
merged 6 commits into from
Apr 2, 2024

Conversation

MRegeard
Copy link
Member

Split of PR #4699. Here is the base class part.

This introduces a base class for object of the pulsar catalog of Fermi (both 2PC and 3PC).

The part where I modify the core class SourceCatalog could be extracted into another base class, say SourceCatalogPC, but it will add another level of inheritance. I don't have a strong opinion on that.

Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard MRegeard added this to the 1.3 milestone Jan 30, 2024
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: Patch coverage is 30.61224% with 68 lines in your changes are missing coverage. Please review.

Project coverage is 75.11%. Comparing base (6616660) to head (663e79e).
Report is 313 commits behind head on main.

Files Patch % Lines
gammapy/catalog/fermi.py 20.00% 56 Missing ⚠️
gammapy/catalog/core.py 57.14% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5056      +/-   ##
==========================================
- Coverage   75.24%   75.11%   -0.13%     
==========================================
  Files         232      234       +2     
  Lines       34318    34944     +626     
==========================================
+ Hits        25822    26248     +426     
- Misses       8496     8696     +200     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This was referenced Jan 30, 2024
Copy link
Contributor

@QRemy QRemy 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, maybe some of the code in the base class could be moved in private functions that can be redefined in the sub classes.

gammapy/catalog/core.py Outdated Show resolved Hide resolved
gammapy/catalog/fermi.py Show resolved Hide resolved
gammapy/catalog/core.py Outdated Show resolved Hide resolved
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
Copy link
Member

@bkhelifi bkhelifi 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 ... In addition to my small comments, there is no dedicated test to these new functionalities?

gammapy/catalog/core.py Show resolved Hide resolved
gammapy/catalog/core.py Show resolved Hide resolved
gammapy/catalog/core.py Outdated Show resolved Hide resolved
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
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 for the effort @MRegeard.
Do some tests need to be added for this?

Also a correction throughout: dictionnary --> dictionary

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 . This is nice. I have left a number of comments inline

gammapy/catalog/core.py Outdated Show resolved Hide resolved
gammapy/catalog/core.py Outdated Show resolved Hide resolved
gammapy/catalog/core.py Outdated Show resolved Hide resolved
gammapy/catalog/fermi.py Outdated Show resolved Hide resolved
gammapy/catalog/fermi.py Show resolved Hide resolved
gammapy/catalog/fermi.py Show resolved Hide resolved
gammapy/catalog/core.py Outdated Show resolved Hide resolved
gammapy/catalog/core.py Outdated Show resolved Hide resolved
Comment on lines 297 to 298
def _get_spectral_table_source_name_key(self):
return self._source_name_key
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem necessary. The result is longer than the direct call.
If this is there for daughter classes, maybe it could be a simple attribute rather than a function?

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 honestly cannot remember why I did that.

Comment on lines +306 to +308
names = [_.strip() for _ in self.spectral_table[source_name_key]]
idx = range(len(names))
return dict(zip(names, idx))
Copy link
Contributor

Choose a reason for hiding this comment

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

The code is very similar between _lookup_spectral_source_idx and _lookup_extended_source_idx. It could be turned a single function taking either self.spectral_table[source_name_key] or self.extended_sources_table[self._source_name_key]as argument

Signed-off-by: Maxime Regeard <regeard@apc.in2p3.fr>
@MRegeard
Copy link
Member Author

Thanks for the effort @MRegeard. Do some tests need to be added for this?

Also a correction throughout: dictionnary --> dictionary

@Astro-Kirsty, I don't think test are needed because the base class is never other than by the daughter classes. So the tests happen in the daughter class. That is also the scheme for the other Fermi base class.

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 . Looks good to me.

@registerrier registerrier merged commit 036fcdc into gammapy:main Apr 2, 2024
14 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
gammapy.catalog
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

5 participants