LSSTDESC / Monitor

Extract light curves for time-variable cosmological objects
BSD 3-Clause "New" or "Revised" License
3 stars 1 forks source link

Prevent variable leaks #35

Closed rbiswas4 closed 8 years ago

rbiswas4 commented 8 years ago

Added an all variable to prevent leakage of imports into the Monitor Namespace. This should fix #34

coveralls commented 8 years ago

Coverage Status

Coverage increased (+1.1%) to 40.845% when pulling ecabefe9aa05d65dad21cb3142fcc9bd02f01ebb on prevent_variable_leaks into cfb4a755ffec04a51edd295c44baa3792736f3bb on master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+1.1%) to 40.845% when pulling 0549f125cbf4dc7a250780498ed7548ee82699d1 on prevent_variable_leaks into cfb4a755ffec04a51edd295c44baa3792736f3bb on master.

jchiang87 commented 8 years ago

I updated the __all__ variables, so that all of the desired components are imported from each module. I think we should get rid of the Monitor class (and its tests) since it's currently just the boilerplate example code that the package was created with. Or maybe we should figure out what that Monitor class is supposed to do and implement it. @rbiswas4 @drphilmarshall @jbkalmbach Thoughts?

coveralls commented 8 years ago

Coverage Status

Coverage increased (+1.1%) to 40.845% when pulling e1d2f6d8e63e106a59800264c34e6b4648a7afe2 on prevent_variable_leaks into cfb4a755ffec04a51edd295c44baa3792736f3bb on master.

rbiswas4 commented 8 years ago

@jchiang87 Thanks for fixing the functions I had missed out. I think @drphilmarshall put in the Monitor as example code. Maybe we should move it to docs if it is not really performing any function?

In the meantime, trying to put in the __all__ variable I realized that some code that I had on my computer had never been committed and hence never been seen by anyone. In fact when I was talking about outputing the reference light curves to the LightCurve class, last week this is what I thinking about.

The basic idea of the this class is supposed to be that once you populate the light curve (whether it is from reference truths, from a data repository or from old data, it should be able to do all the possible computations/transformations you might want to do.

drphilmarshall commented 8 years ago

That's right. If you want to monitor objects, you need a Monitor Object to do it for you! See my previous email on the python lines I had imagined us wanting to write. Feel free to go ahead and remove the monitor class for now - but if we start proliferating scripts that do various things with ensembles of objects and their light curves, we might want to resurrect the notion.

On Mon, Jun 20, 2016 at 5:23 PM, rbiswas4 notifications@github.com wrote:

@jchiang87 https://github.com/jchiang87 Thanks for fixing the functions I had missed out. I think @drphilmarshall https://github.com/drphilmarshall put in the Monitor as example code. Maybe we should move it to docs if it is not really performing any function?

In the meantime, trying to put in the all variable I realized that some code that I had on my computer had never been committed and hence never been seen by anyone. In fact when I was talking about outputing the reference light curves to the LightCurve class, last week this is what I thinking about.

The basic idea of the this class is supposed to be that once you populate the light curve (whether it is from reference truths, from a data repository or from old data, it should be able to do all the possible computations/transformations you might want to do.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DarkEnergyScienceCollaboration/Monitor/pull/35#issuecomment-227308124, or mute the thread https://github.com/notifications/unsubscribe/AArY98-PwC-g4aKRBwjwhrg6PezVGE7Gks5qNy78gaJpZM4I5Cto .

rbiswas4 commented 8 years ago

I think it will be there eventually be in when we have a piece of code that will return a list of possible sources for an objectID given search parameters including [].

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.8%) to 32.386% when pulling d92da04ed922a908bd2460960cd54695989de62e on prevent_variable_leaks into ff4584db9b0132e701d3859aa1806122d424f648 on master.

rbiswas4 commented 8 years ago

OK. Unless anyone has any objections, I will merge this branch.

BTW, since there were conflicts I rebased but had to force push. Is there a preference for this vs merging to master on the terminal?