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

ISSUE #134 Support ECOStress Spectral Library #153

Closed
wants to merge 14 commits into from
Closed

ISSUE #134 Support ECOStress Spectral Library #153

wants to merge 14 commits into from

Conversation

bdegley4789
Copy link
Member

@lewismc I think this code addresses #134 .
The EcoStress Spectral Library is almost the same format as ASTER. To make it the same as ASTER you just need to add 5 ‘\n’ after the description to every .spectrum.txt file. We can extract this into its own class and write test cases later. For now I just have it as an example script to demo.

Downloading EcoStress can take awhile for your request to go through.
If you want to run on your own system download and unzip the zip I staged in here to pycoal/examples then just run.
python example_ecoStress.py ecospeclib-1525266883141

@coveralls
Copy link

coveralls commented May 11, 2018

Coverage Status

Coverage remained the same at 68.526% when pulling f4c7a23 on bdegley4789:master into 9b12371 on capstone-coal:master.

Copy link
Member

@lewismc lewismc 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 taking this one @bdegley4789 please consider my comments.

dir[:] = [d for d in dir]
for items in fnmatch.filter(files, "*spectrum.txt"):
#Remove vegation files because their format causes errors in the Spectral class
if "vegetation" in items:
Copy link
Member

Choose a reason for hiding this comment

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

Can you remind me again why we are not using the vegetation indexes?

Copy link
Member Author

Choose a reason for hiding this comment

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

On lines 4 & 5 Vegetation Spectra has Genus and Species instead of Subclass and Particle Size entries like other Spectra.
The ASTER conversion function throws an error when it reads lines 4 & 5 so I just removed the vegetation files.
If we wanted to read Vegetation files then we would need to change lines 4 & 5 to give them a subclass and particle size with a default value like I did with Spectral 7 conversion function. I know you said you prefer if we not plug in default values. Let me know if you want me to change this, it wouldn't be very hard

Copy link
Member Author

Choose a reason for hiding this comment

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

manmade.concrete.constructionconcrete.solid.all.0598uuucnc.jhu.becknic.spectrum.txt

Name: Construction Concrete
Type: manmade
Class: Concrete
Subclass: Construction Concrete
Particle Size: Solid
Sample No.: 0598UUUCNC

Copy link
Member Author

Choose a reason for hiding this comment

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

nonphotosyntheticvegetation.bark.abies.concolor.all.vh311.ucsb.asdnicolet.spectrum.txt

Name: Abies concolor bark
Type: non photosynthetic vegetation
Class: bark
Genus: Abies
Species: concolor
Sample No.: VH311

Copy link
Member

Choose a reason for hiding this comment

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

OK, ideally we want to be able to use every spectra from the entire ECOSTRESS library. If you could implement the suggested workaround for including the vegetation files it would be appreciated.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lewismc Ok, I fixed it so it works with Vegetation files. I added the Subclass and Particle Size and removed the Genus and Species. It can now convert the entire EcoStress library

input_file.write(contents)
input_file.close()

ecoStress = mineral.AsterConversion()
Copy link
Member

Choose a reason for hiding this comment

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

Please change ecoStress to ecostress, thanks

Copy link
Member

Choose a reason for hiding this comment

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

Once you address this then we can think about ripping this code out of the example and embedding it in conversion.py under a new Class the same as the other conversion functions.

files of `ECOStress Spectral Library <https://speclib.jpl.nasa.gov/>`_

Dependencies
`ECOStress Spectral Library <https://speclib.jpl.nasa.gov/>`_
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be more descriptive here.
The downloads page includes the following artifacts

All Lunar (17 files)
All Man-made (72 files)
All Meteorites (59 files)
All Minerals (3104 files)
All Non-Photosynthetic Vegetation (62 files)
All Rock (647 files)
All Soil (120 files)
All Vegetation (1031 files)
All Water (9 files)

Or

ALL Spectra (5104 files)

Which of the above do we download?

Copy link
Member Author

Choose a reason for hiding this comment

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

ALL Spectra, I have now made a comment in the code to specify

# Floor, Boston, MA 02110-1301, USA.

Mineral Classification API
*********************************
Copy link
Member

Choose a reason for hiding this comment

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

You need to add the following

.. autoclass:: pycoal.mineral.SpectalToAsterFileFormat
   :special-members:
   :members:

.. autoclass:: pycoal.mineral.FullSpectralLibrary7Convert
   :special-members:
   :members:

@lewismc
Copy link
Member

lewismc commented May 12, 2018

Hi @bdegley4789 I pushed some improvements, when you have time, please resolve the conflict. Sorry about that.

@bdegley4789
Copy link
Member Author

All good, it's resolved. I'm not sure what it was. It looks like it was the same thing I added to mineral.rst

@bdegley4789
Copy link
Member Author

I'm not sure why the travis ci failed on python 3.3 but passed on all the others. It looks like the commit I did for EcoStress passed though since it has the green check. Must be something with the merge

.. autoclass:: pycoal.mineral.AsterConversion
:special-members:
:members:

Copy link
Member

Choose a reason for hiding this comment

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

This can now be removed as the classes are present within thr new conversion.py file.

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.

3 participants