spice-herald / RQpy

This repository provides helpful tools for DM search analysis using detectors studied by the Pyle Group.
GNU General Public License v3.0
4 stars 3 forks source link

get rid of scdmsPyTools? #152

Closed cwfink closed 4 years ago

cwfink commented 4 years ago

So I heard in passing that the IO functionality of scdmsPyTools has now officially been placed in its own module. We should look into just using this new functionality and remove scdmsPyTools as a dependency for opening CDMS data.

slwatkins commented 4 years ago

Possibly, though they made it kind of a mess to use it I think.

http://titus.stanford.edu:8080/git/summary/?r=DataHandling/pyRawIO.git

It looks like you need to define a path to a BatCommon installation before building pyRawIO. Not hard, but just kind of confusing/annoying.

serfass commented 4 years ago

I think switching to pyRawIO.git is a must as it is not longer in scdmsPyTools. The code is the same but BatCommon and IOLibrary are now external libraries (compiled using cmake) and thus need environmental variables to point to their location. There is no need to install pyRawIO (and other cdms libraries) at SLAC, rather use "global release” on “centos7” (best) or "rhel6-64” (source /nfs/slac/g/supercdms/packages/globalrelease/setup_cdms.sh V02-04-00) or Jupiter lab. We should make sure that at least QETpy is installed with the global release (not sure the status).

Also the code in RQpy to access information using scdmsPytTools/pyRawIO is rather old so we will need to update
it for the newest Midas format. I can help with that next week.

On Apr 27, 2020, at 10:29 AM, Samuel Watkins notifications@github.com wrote:

Possibly, though they made it kind of a mess to use it I think.

http://titus.stanford.edu:8080/git/summary/?r=DataHandling/pyRawIO.git http://titus.stanford.edu:8080/git/summary/?r=DataHandling/pyRawIO.git It looks like you need to set up a path in your .bashrc or .cshrc that points to a BatCommon installation before building pyRawIO. Not hard, but just kind of confusing.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ucbpylegroup/RQpy/issues/152#issuecomment-620125878, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARMS2JB25CANE5YQJQC7MLROW6I5ANCNFSM4MQPDJ7A.

pibion commented 4 years ago

Hi all, this is still an issue and is blocking people from using this tool on CDMS analysis clusters.

From @bloer:

When you import rqpy, it looks to see if scdmsPyTools is installed (which it will be in the cdms environment), and then tries to import scdmsPyTools.BatTools.*, which fails because we moved all the raw file IO stuff into the rawio package.

Does anyone have some time to work on this? Do people have access to the environments?

serfass commented 4 years ago

I don't mind spending a couple of days on that early next week. Besides switching to pyRawIO, there are some updates needed for the newest midas format. Also the low pass filter gain is hardcoded to 2 in several places. We can't properly use RQpy currently for CUTE data for example without some modifications.

serfass commented 4 years ago

@bloer thanks! The midas io functionality will still need to be updated to be usable with newest midas raw data. I can take care of updating and testing it this weekend

serfass commented 4 years ago

I updated "_process_iv_didv.py" to work with RevE data and back compatible with older data format. The code requires a new version of pyRawIO (1.2.3). I removed "lgcHV" argument (not needed) but added "lowpassgain" parameters (all default values changed to RevE...)

So far I tested functionality with a CUTE PD2 dataset and SLAC R51 TES chips IV/dIdV dataset. I only checked that the correct parameters (QET bias, signal amp/freq, ndarray index) are being used as well as checking that the offset and cut efficiency are reasonable. I did not do a full IV / dIdV processing or check dIdV fit results. I can do that for CUTE data. If you have a pickle file from R51 single file processing, I could do quick a comparisons.

I did not test other functions such that uses rawIO such as load, sim.

serfass commented 4 years ago

Just to be clear. I am using feature/rawio

slwatkins commented 4 years ago

Hi All,

As one of the hurdles to merging these changes to master was that the JupyterHub doesn't seem to have rawio installed anywhere, I went back and made the IO functions backwards compatible with the old scdmsPyTools as a stopgap measure. So, merging these changes to master won't break rqpy for people using the old scdmsPyTools. I've added a deprecation warning for this, with the intent of removing the scdmsPyTools dependency all together when rawio is available/simple-to-use on the JupyterHub.

If everyone is happy with this, I think we can merge soon. Though, I would appreciate it if someone could make sure the rawio-based functionality still works.

serfass commented 4 years ago

Hi Sam,

The rqpy/process/_processing_iv_didv.py code won't work with old scdmsPyTools on JupyterHub in any cases. It has been updated to work with latest RevE midas raw data and requires latest pyRawIO version 1.2.3.

Also "JupyterHub" is going to be completely deactivated very soon. You should probably switch now to new sdf onDemand server.

Perhaps we should have a short discussion on zoom tomorrow or Monday with Caleb and Ben on how to proceed?

slwatkins commented 4 years ago

Ah, okay. Well, I've emailed Yee about getting a home directory. Once I'm set up and and using rawio , then I will undo my changes and merge this branch. I'm not sure a meeting is necessary just yet - I think we just want a quick check to make sure everything makes sense once @cwfink and I are set up to be able to test.

serfass commented 4 years ago

I am testing a bit more the code with some actual processing. I am a little confused. In the _proces_iv_didv.py, the following parameters are being accessed from QETpy DIDV: didvmean = didvobj.didvmean didvstd = didvobj.didvstd

However didvmean and didvstd don't seem to exist. Looking at QETpy it seems to be "_didvmean" and "_didvstd" (and I don't see an @property somewhere to return didvmean). What I am missing? Is the current RQpy master working with master QETpy?

cwfink commented 4 years ago

Hi Bruno, good catch. there was a major refactoring of the DIDV code in QETpy a month or so ago https://github.com/ucbpylegroup/QETpy/pull/99 and the mean and std were made to be 'private'. I think the processing code should be fine if didvmean is simply replaced with _didvmean. Unfortunately there are a few other non backwards compatible changes as well, so the IVAnalysis class will need to be updated too. I'll start doing this soon

serfass commented 4 years ago

Hi Caleb,

I think it would cleaner to modify QETpy DIDV class, adding

@property def didvmean(self): return self._didvmean

and if needed:

@didvmean.setter def didvmean(self,value): self._didvmean = value

so (1) we don't need to modify RQpy and (2) using "private" data (and functions) outside the scope defeat a bit the purpose (though I grant they are not really private in the C++ sense).

cwfink commented 4 years ago

I think a case could be made for changing the name from _didvmean to didvmean to not have to change the processing, but other methods have changed too, so we would need to change some of the analysis tools in RQpy regardless, so not sure it would matter.

regarding 'getter and setter' methods for these values, in this case I feel that makes the code much more unreadable and is not really common practice for situations like this.

The motivation behind making these particular variables 'private' was to indicate that this is not a variable that is intended to be commonly used in standard usage of the DIDV class, but there is no important reason that the user should not have access to it if they want. Also, by making them 'hidden' they do not show up when tab-completing (at least in certain editors) so it makes the usage a bit cleaner. The way we are using them in the processing is a bit of a 'one off' for this class, as we are just trying to save time when actually fitting the DIDV after processing of an IV/DIDV sweep.

serfass commented 4 years ago

Sounds good thanks. I probably have a more strict view of what "private" variable should be because I write mainly C++ code and I find the python's "property decorator" very useful to maintain oriented object structure and quite readable (but shouldn't be abused I guess).