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

Created method to retrieve the wavelength lines from NIST. #30

Closed
wants to merge 6 commits into from
Closed

Created method to retrieve the wavelength lines from NIST. #30

wants to merge 6 commits into from

Conversation

b1quint
Copy link

@b1quint b1quint commented Jun 20, 2018

Created method to handle what was requested on Issue #28.

This method uses BeautifulSoup4 instead of HTMLParser. It also uses NumPy and Pandas.

@tepickering
Copy link
Contributor

tepickering commented Jun 21, 2018

looks like pandas needs to be added to the travis dependency list...and BeautifulSoup...

@@ -1,4 +1,4 @@
name: packagename
name: dummy
Copy link
Member

Choose a reason for hiding this comment

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

use specreduce here rather than dummy

@b1quint
Copy link
Author

b1quint commented Jun 22, 2018

Three tests still fail when trying to import pandas. I modified the setup.cfg and the .rtd-environment.yml files to include pandas and beautifulsoup4. I am trying to find where else should I add them to have all tests passing.

@bsipocz
Copy link
Member

bsipocz commented Jun 22, 2018

@b1quint - If they are dependencies, the they should be added to the travis config file into the CONDA_DEPENDENCIES env variable

@b1quint
Copy link
Author

b1quint commented Jun 22, 2018

\o/

@tepickering
Copy link
Contributor

tepickering commented Jun 22, 2018

i'll restate my comment from #29 here: a disadvantage of returning DataFrames is the lack of any units metadata. pandas will likely not have any units support for a while as well (pandas-dev/pandas#15698). i think it's much more preferable to attach explicit units metadata to wavelengths than rely on a convention. this is why astropy.units was created, after all. there might be a way to hack the metadata into the DataFrame, but i think it's a lot more straightforward to use Quantity arrays or Qtable. They are also much more compatible with the rest of the astropy stack.

@bsipocz
Copy link
Member

bsipocz commented Jun 22, 2018

I think this package should use astroquery to access the NIST database rather than reinvent and implement the wheel. If any functionality is missing from astroquery, contribute it to there.

https://astroquery.readthedocs.io/en/latest/nist/nist.html

@tepickering
Copy link
Contributor

i completely agree! had no idea NIST was already implemented in astroquery....

@bsipocz
Copy link
Member

bsipocz commented Jun 22, 2018

Not just NIST, but also a few other ones (that may not be relevant here, though): https://astroquery.readthedocs.io/en/latest/#other

@b1quint
Copy link
Author

b1quint commented Jun 22, 2018

If this is the case, I will just close this Pull Request. Is that ok?

@b1quint b1quint closed this Jun 22, 2018
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.

None yet

3 participants