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

Feature/collections class #91

Merged
merged 40 commits into from Dec 14, 2016

Conversation

granrothge
Copy link
Contributor

Provides a start of scans collections class that allows pcolor plots and waterfall plots of multiple runs.

@pseudocubic
Copy link
Contributor

First of all, thanks for the PR! I think that this will make a good addition to neutronpy.

I have briefly looked over it and my first impression is good. I will spend some time with it next week to give more complete comments and maybe submit some suggested changes to you. A couple of things that captured my attention are the docstrings, which will have to be complete before a merge, the class definition, which should be capitalized and inherit object (i.e. the "new" Python 3 style class definition), i.e. Scans(object), and the name of the package.

To address the last point, I'm not sure if scancollection is the right name (at the very least, to be consistent with the rest of the package it should be scan_collection). With the single class there it might be more appropriate as a module within the data package, but if we can foresee collections of other objects, like materials or instruments for example, then maybe a package named something like containers (collections would have been my choice, but better to not overlap with the standard library), with the submodule scans like you have it. I am certainly open to suggestions/discussion on this point.

@codecov-io
Copy link

codecov-io commented Dec 2, 2016

Current coverage is 84.32% (diff: 89.18%)

Merging #91 into master will increase coverage by 0.12%

@@             master        #91   diff @@
==========================================
  Files            44         45     +1   
  Lines          3780       3854    +74   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3183       3250    +67   
- Misses          597        604     +7   
  Partials          0          0          

Powered by Codecov. Last update 983fe90...2a29af8

@granrothge
Copy link
Contributor Author

As to the questions of naming I can change it to scan_collection. I can not think of a case where one would like a collection that can not be fit by the scans_collection and the scan. Say for an example if I wanted to do a waterfall plot as function of doping concentration. I think I would add a column for doping to the scan, then a variation by material would then be a scan collection. I think a collection of objects that would contain scans and other things might be better handled by a higher level collection yet, (maybe a NOSQL database), because your processing chain would likely be vary different. Say for example if you wanted to constrain an sqw model to both scan data and a magnetic susceptibility measurement, I think that putting the susceptibility data into a scan would add a lot of overhead.

I'll look into improving the doc strings.

@granrothge
Copy link
Contributor Author

Let me know how you want the module named and I will make the change. I think the doc string are now ready to go.

@pseudocubic
Copy link
Contributor

Thanks for updating the docstrings. Could you also add a one-liner in the top level docstring of the file?

The class definition should also be changed to class Scans(object):

After considering it some more, I think that this would would be better as an addition to the data package, i.e. in neutronpy/data/scans.py and add from .scans import Scans in the __init__.py. I would still keep the tests in a separate file like you have it (but rename to test_scans.py), instead of combining it with the data tests.

Could you also rebase to make a more simple commit history?

Otherwise I think this is pretty close to being ready for merge. I will take care of merging the docstrings into the sphinx-built documentation, and updating the changelog myself once we have merged.

David M Fobes and others added 12 commits December 14, 2016 09:43
Added a conda-recipe and updated installation documentation.
Added automatic building of conda packages on tagged releases
Bash if then statement not correct for auto conda build

updated anaconda cloud neutronpy version

anaconda-client is required to upload the automatic builds.

Added encrypted token for anaconda cloud upload
updated encrypted token for anaconda cloud upload
@granrothge
Copy link
Contributor Author

Ok so this should be good now.

@pseudocubic
Copy link
Contributor

The directory neutronpy/scancollection and the file tests/test_scancollection.py are still left over after your changes, but once those are gone I should have no problem to merge the PR.

@granrothge
Copy link
Contributor Author

the combination of the rebase, fetch and merge, brought things back that I had removed. Sorry. It should be good now.

@granrothge
Copy link
Contributor Author

Finally, this should be ready.

@pseudocubic pseudocubic merged commit 43d3012 into neutronpy:master Dec 14, 2016
@pseudocubic
Copy link
Contributor

pseudocubic commented Dec 14, 2016

Thanks for the contribution and all your hard work! I have some minor bug fixes to include, and need to make sure the docs get generated, but hopefully I can get an incremental release onto the anaconda cloud in a few days.

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