Adds a Firefly particle group generator

[zhafen]

Based on SpectrumGenerator. Happy to clean up as needed for it to be mergable.

Looks like my basic linetools code got mixed in too... Best to remove that.

[chummels]

This is some cool functionality that you've written for Trident--thanks, Zach! I think it will be super great to use this with FIREFly. A few changes requested to get this in, but I don't think anything too onerous.

1. Strip out the LineTools changes in the code, which I believe are unrelated to the content of this PR. We'll get the LineTools stuff addressed in that other PR soon.
2. Add a little bit in the way of docstrings for your new class--again, this does not need to be too exhaustive, but just generally describing the function, args, and a bit of what is going on in the code.
3. Similar to this, I was trying to think where in the narrative docs we can mention this, just so people will know that this cool functionality exists. Since this is one of the first instances of us integrating with another code, perhaps we just include an entry in the FAQ or something and just provide an example of how to use this code with FIREFly? Not sure where else we could put it, but I'm open to suggestions. Or maybe even adding a new page in the docs for "Visualizing Sightlines with the FIREFly Viz Tool" or something: https://trident.readthedocs.io/en/latest/ . Either way, it would be really nice to just show the user how to use this functionality on a dataset and how it looks in FIREFly.
4. A basic unit test test_fire_fly_generator.pyin src/trident/tests that just tests to make sure the new functionality doesn't fail when called on a sample dataset.

Let me know if you need help with any of this, and I'll see what I can do. Thanks again for this awesome contribution!

[chummels]

Remove commented out docstrings for SpectrumGenerator and replace with description of FireFlyGenerator functionality and arguments.

[chummels]

I believe this is leftover in your stack from previous changes you've made on LineTools. Is it possible to strip this out so we just focus on the Firefly functionality?

[chummels]

I believe this is leftover in your stack from previous changes you've made on LineTools. Is it possible to strip this out so we just focus on the Firefly functionality?

[chummels]

I believe this is leftover in your stack from previous changes you've made on LineTools. Is it possible to strip this out so we just focus on the Firefly functionality?

[chummels]

I'm guessing that this functionality will only work for particle-based datasets, right? If so, perhaps we need a conditional somewhere at the beginning of this code that checks to see if this is particle-based or grid-based and raises a "Not Implemented error" for grid-based datasets? Or something like that.

[zhafen]

Would be happy to make these changes. Thanks for identifying what needs to be done!

On Mon, Aug 1, 2022 at 9:58 AM Cameron Hummels ***@***.***> wrote: ***@***.**** requested changes on this pull request. This is some cool functionality that you've written for Trident--thanks, Zach! I think it will be super great to use this with FIREFly. A few changes requested to get this in, but I don't think anything too onerous. 1. Strip out the LineTools changes in the code, which I believe are unrelated to the content of this PR. We'll get the LineTools stuff addressed in that other PR soon. 2. Add a little bit in the way of docstrings for your new class--again, this does not need to be too exhaustive, but just generally describing the function, args, and a bit of what is going on in the code. 3. Similar to this, I was trying to think where in the narrative docs we can mention this, just so people will know that this cool functionality exists. Since this is one of the first instances of us integrating with another code, perhaps we just include an entry in the FAQ or something and just provide an example of how to use this code with FIREFly? Not sure where else we could put it, but I'm open to suggestions. Or maybe even adding a new page in the docs for "Visualizing Sightlines with the FIREFly Viz Tool" or something: https://trident.readthedocs.io/en/latest/ . Either way, it would be really nice to just show the user how to use this functionality on a dataset and how it looks in FIREFly. 4. A basic unit test test_fire_fly_generator.pyin src/trident/tests that just tests to make sure the new functionality doesn't fail when called on a sample dataset. Let me know if you need help with any of this, and I'll see what I can do. Thanks again for this awesome contribution! ------------------------------ In trident/firefly_generator.py <#184 (comment)> : > +from yt.utilities.on_demand_imports import \ + _firefly + +from trident.config import \ + ion_table_dir, \ + ion_table_file, \ + ion_table_filepath +from trident.ion_balance import \ + add_ion_number_density_field, \ + atomic_mass +from trident.line_database import \ + LineDatabase + +class FireflyGenerator( object ): + """ + ## OLD TEXT COMMENTED OUT ## Remove commented out docstrings for SpectrumGenerator and replace with description of FireFlyGenerator functionality and arguments. ------------------------------ In tests/test_line_database.py <#184 (comment)> : > @@ -62,6 +62,14 @@ def test_line_database_from_input(): assert ld.lines_all[0].identifier == HI.identifier print(ld) +def test_line_database_add_line_linetools(): I believe this is leftover in your stack from previous changes you've made on LineTools. Is it possible to strip this out so we just focus on the Firefly functionality? ------------------------------ In trident/ion_balance.py <#184 (comment)> : > @@ -245,7 +245,8 @@ def add_ion_fields(ds, ions, ftype='gas', # If line_database is set, then use the underlying file as the line list # to select ions from. if line_database is not None: - line_database = LineDatabase(line_database) I believe this is leftover in your stack from previous changes you've made on LineTools. Is it possible to strip this out so we just focus on the Firefly functionality? ------------------------------ In trident/line_database.py <#184 (comment)> : > @@ -147,8 +148,8 @@ def __init__(self, input_file=None): else: self.input_file = 'Manually Entered' - def add_line(self, element, ion_state, wavelength, gamma, - f_value, field=None, identifier=None): + def add_line(self, element, ion_state, wavelength, gamma=None, I believe this is leftover in your stack from previous changes you've made on LineTools. Is it possible to strip this out so we just focus on the Firefly functionality? ------------------------------ In trident/firefly_generator.py <#184 (comment)> : > + # angstrom spectral bins from 2000-4000 angstroms, then post-process by + # adding a MW foreground a QSO background at z=0.5 and add a boxcar line + # spread function of 100 angstroms width. Plot it and save the figure to + # 'spec_final.png'. + + # >>> import trident + # >>> ray = trident.make_onezone_ray(redshift=0.5) + # >>> sg = trident.SpectrumGenerator(lambda_min=2000, lambda_max=4000, + # ... dlambda=1) + # >>> sg.make_spectrum(ray) + # >>> sg.add_qso_spectrum(emitting_redshift=.5) + # >>> sg.add_milky_way_foreground() + # >>> sg.apply_lsf(function='boxcar', width=100) + # >>> sg.plot_spectrum('spec_final.png') + """ + def __init__(self, line_database='lines.txt', I'm guessing that this functionality will only work for particle-based datasets, right? If so, perhaps we need a conditional somewhere at the beginning of this code that checks to see if this is particle-based or grid-based and raises a "Not Implemented error" for grid-based datasets? Or something like that. — Reply to this email directly, view it on GitHub <#184 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCVZBFOENVDND7MA2UURTDVW76Z7ANCNFSM55GCQL3Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Dr. Zachary Hafen (he/him/his) McCue Fellow Center for Cosmology UC Irvine