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

Emsystem #416

Merged
merged 30 commits into from
Oct 24, 2017
Merged

Emsystem #416

merged 30 commits into from
Oct 24, 2017

Conversation

tiffanyhsyu
Copy link
Collaborator

No description provided.

Copy link
Contributor

@profxj profxj left a comment

Choose a reason for hiding this comment

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

Need docs and more tests.

Returns
-------
"""
if method == 'PG16':
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a http link to the paper here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This has been added


Parameters
----------
alis_file
Copy link
Contributor

Choose a reason for hiding this comment

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

add

def __init__(self, radec, zem, vlim=None, em_type=None, name=None):

self.zem = zem
if vlim is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to use the zLimits Class here.

I can help set that up. Remind me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now in Issue #419

#
return test

def chk_emline(self, component):
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use components with EmLine (at least not now).

Remove components and/or this method.

Same goes for anywhere else you see components.

self._ionN = ltiu.iontable_from_components(self._components, **kwargs)

def fill_trans(self, **kwargs):
""" Fills the ionN Table from the list of components
Copy link
Contributor

Choose a reason for hiding this comment

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

list of emlines

else:
return [self._emlines[ii] for ii in mt]

def get_component(self, inp):
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this method

return None

def measure_restew(self, spec=None, **kwargs):
""" Measure rest-frame EWs for lines in the AbsSystem
Copy link
Contributor

Choose a reason for hiding this comment

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

EmSystem

vlim = self.vlim
ltap.stack_plot(self.list_of_abslines(), vlim=vlim, **kwargs)

def to_dict(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just checked that this works! There is also a test for it now.

data_dir = os.path.join(os.path.dirname(__file__), 'files')
return os.path.join(data_dir, filename)


Copy link
Contributor

Choose a reason for hiding this comment

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

Add some other, simpler tests for instantiating the object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added a test for creating an EmSystem object


def test_emline_from_alis():
lio.emlines_from_alis_output(data_path('spec1d_J0018p2345_KASTb_coadd.mod.out'))

Copy link
Contributor

Choose a reason for hiding this comment

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

Add some checks that this actually worked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@profxj
Copy link
Contributor

profxj commented Oct 13, 2017

Hey @tiffanyhsyu -- One test is still failing:


def test_emline_from_alis():
    J0018_emlines = lio.emlines_from_alis_output(data_path('spec1d_J0018p2345_KASTb_coadd.mod.out'))

    assert len(J0018_emlines) == 8
    np.testing.assert_allclose(J0018_emlines[0].z, 0.01540425)
    np.testing.assert_allclose(J0018_emlines[1].wrest.value, 4341.684) #Hgamma

That is because the file is not in the repo. And you may need to edit one
other thing too. But add that file in first. Thanks

@profxj
Copy link
Contributor

profxj commented Oct 13, 2017

The test, by the way, is

tests/test_io.py

@tiffanyhsyu
Copy link
Collaborator Author

tiffanyhsyu commented Oct 13, 2017

@profxj I've added a 'files' folder under 'test' and uploaded the ALIS .mod.out file there -- is that ok, as far as the organization of the files?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 71.735% when pulling ee25b81 on emsystem into 9ff0b74 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 71.735% when pulling d1a1fca on emsystem into dd5c4a8 on master.

@profxj
Copy link
Contributor

profxj commented Oct 24, 2017

merging!

@profxj profxj merged commit 5ebd872 into master Oct 24, 2017
@profxj profxj deleted the emsystem branch October 24, 2017 12:18
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