Porting the line ID tool from tardisanalysis #1890
Conversation
jamesgillanders
requested review from
marxwillia,
jaladh-singhal and
MarkMageeAstro
|
Before a pull request is accepted, it must meet the following criteria:
|
|
I moved this tool and the opacities.py tool to a folder titled 'analysis_tools' - that's why 2 new files are appearing instead of 1 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1890 +/- ##
==========================================
- Coverage 67.80% 62.55% -5.26%
==========================================
Files 177 68 -109
Lines 14534 7127 -7407
==========================================
- Hits 9855 4458 -5397
+ Misses 4679 2669 -2010 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you so much @jamesgillanders - this looks like a nice addition. I've few comments - let me know if you need help with any of it.
Besides, could you please create a notebook (or rst file) to demonstrate the use of this tool so that it shows up on docs. Having unit-tests will also be nice but I think we might wanna save them for another PR.
tardis/analysis_tools/line_ID.py
Outdated
| f = open(output_filename, "w") | ||
| f.write( | ||
| f"# Line Transitions in Wavelength Range " | ||
| f"{self.lam_min.value:.1f} - {self.lam_max.value:.1f}" | ||
| f" Angstroms\n" | ||
| ) | ||
| dataframe.to_csv(f, sep="\t", index=False) | ||
| f.close() |
There was a problem hiding this comment.
| f = open(output_filename, "w") | |
| f.write( | |
| f"# Line Transitions in Wavelength Range " | |
| f"{self.lam_min.value:.1f} - {self.lam_max.value:.1f}" | |
| f" Angstroms\n" | |
| ) | |
| dataframe.to_csv(f, sep="\t", index=False) | |
| f.close() | |
| with open(output_filename, "w") as f: | |
| f.write( | |
| f"# Line Transitions in Wavelength Range " | |
| f"{self.lam_min.value:.1f} - {self.lam_max.value:.1f}" | |
| f" Angstroms\n" | |
| ) | |
| dataframe.to_csv(f, sep="\t", index=False) |
It's cleaner to do file operations this way. It ensures auto-closing.
tardis/analysis_tools/line_ID.py
Outdated
| dataframe_collapsed = dataframe.groupby(["Species"]).sum() | ||
| dataframe_collapsed = dataframe_collapsed.sort_values( | ||
| by=["Total no. of transitions"], ascending=False | ||
| ) |
There was a problem hiding this comment.
Since this reuses dataframe that we have calculated already in previous if block, let's put it in above block after L245.
tardis/analysis_tools/line_ID.py
Outdated
| f = open( | ||
| f"{output_filename[:-4]}_collapsed{output_filename[-4:]}", "w" | ||
| ) | ||
| f.write( | ||
| f"# Line Transitions in Wavelength Range " | ||
| f"{self.lam_min.value:.1f} - {self.lam_max.value:.1f}" | ||
| f" Angstroms\n" | ||
| ) | ||
| dataframe_collapsed.to_csv(f, sep="\t", index=True) | ||
| f.close() |
There was a problem hiding this comment.
This will move above as well. While doing so, can you please change it to with syntax like I suggested in earlier comment?
| self.lam_max = lam_max | ||
|
|
||
| def plot_summary( | ||
| self, nlines=None, lam_min=None, lam_max=None, output_filename=None |
There was a problem hiding this comment.
IMO we should separate out line contributions file writing from plot_summary() by creating another method like save_summary(). It's not clear to me why it has to be done in plotting function - let me know if I'm missing something
| self._lines_ids = None | ||
| self._lines_ids_unique = None | ||
|
|
||
| @property |
There was a problem hiding this comment.
Can you please add docstring to this non-private property (an others below it) so that docstring coverage pipeline passes?
| def plot_summary( | ||
| self, nlines=None, lam_min=None, lam_max=None, output_filename=None | ||
| ): | ||
|
|
There was a problem hiding this comment.
A docstring should be added here as well, not only because of pipeline but because it's most important function of this class.
tardis/analysis_tools/line_ID.py
Outdated
| from tardis.visualization.tools.sdec_plot import SDECData | ||
|
|
||
|
|
||
| class line_identifier: |
There was a problem hiding this comment.
| class line_identifier: | |
| class LineIdentifier(object): |
blatant PEP 8 violation 🤣
tardis/analysis_tools/opacities.py
Outdated
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class opacity_calculator(object): |
There was a problem hiding this comment.
| class opacity_calculator(object): | |
| class OpacityCalculator: |
|
I will revisit this soon and try to get it to a merge-able state |
|
@jamesgillanders can this still be fixed up to merge? |
|
*beep* *bop* Hi, human. I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently. Please add your name and email to In case you need to map an existing alias, follow this example. |
|
This can just be merged, right? (once requests are addressed) |
Updated a few sections of the code to reflect earlier review comments, and to come in line with PEP8 requirements.
|
*beep* *bop* Hi, human. I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently. Please add your name and email to In case you need to map an existing alias, follow this example. |
As above. Updates mainly revolve around updating the format of the output figure to be "cleaner".
|
*beep* *bop* Hi, human. I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently. Please add your name and email to In case you need to map an existing alias, follow this example. |
Fixing the code to match black requirements
|
Ciao tutti, I have updated the code here to work with the new restructured code base, and to reflect some of the above review comments from ~2 years ago. I don't know why tests are failing, and I don't know if the current code style is TARDIS-y enough. But the code works, and is meant to be a port from the old tardis_analysis repo. I'd be in favour or merging this (assuming you guys can test + run it!) so this feature is available for users. I for one certainly miss it in the new versions! |
|
Updated the constants package in opacities.py to use the internal TARDIS constants package -- good spot Andrew! |
|
@jaladh-singhal Can you re-review? |
This PR moves the tardis_line_id tool across from tardisanalysis to the main TARDIS repository. The TARDIS line ID tool was originally written by @unoebauer.
Description
Code has been updated to work with the newest version of TARDIS, but behaves as before.
The tool can be used to extract a list of the N most prominent transitions (default has N = 20) within a user-specified wavelength range (default is the entire wavelength range of the simulation). This can be particularly helpful for diagnosing the species with the largest contribution within a wavelength range, which perhaps has a blend of different absorption features.
Motivation and context
This PR simply relocates the old tool, and makes it compatible with the new version of TARDIS.
How has this been tested?
Examples
Here is the output plot from the old tool:
test_line_ID_K2.pdf
And here is the new version:
test_output.pdf
Type of change
Checklist