gafusion / omas

Ordered Multidimensional Array Structure
http://gafusion.github.io/omas
MIT License
30 stars 15 forks source link

Add LP data (isat, density, ...) mapping functionality #156

Closed eldond closed 3 years ago

eldond commented 3 years ago
orso82 commented 3 years ago

@eldond I would revert the last commit 4d1612f since 1) I believe that the mapping functions should be as simple and as performant as possible, 2) we do not want a priori to prevent users to mix data from different shots and 3) if we really really want to implement this kind of checks, that should be done at a higher level, for all devices, without having to write checks within each mapping function.

orso82 commented 3 years ago

@eldond the gathering of LP data does not take advantage of requesting multiple MDS+ signals at once. Would you like to merge this as is for now, or will you be working on the faster data fetching in this PR?

eldond commented 3 years ago

@orso82 I'm against having both default shot numbers and no checks at all. I think there should be at least a warning when mixing data. Maybe the data mappers could fail if a shot is recorded in the ODS and the current shot doesn't match that. If there's no shot written in the ODS, they can assume you're making some sort of test or theoretical thought experiment or something and let you do it. We could also take the default shot numbers out and put them all in the regression tests instead.

eldond commented 3 years ago

@orso82 This should be converted to faster MDS access before merging.

orso82 commented 3 years ago

Good point on possible issues arising because pulse has a default value. I did not think about that, mostly because I envisioned that the mapping functions would not be called directly, but only through the 'ODS.open(...)' method, which in fact requires pulse to be passed. But I now understand what you were trying to do, and you are right, that if functions are called directly, having defaults could be a problem.

Once again though, I think the solution should be more general of what done in your latest commit. I have some ideas and will have some fix coming soon.

orso82 commented 3 years ago

@eldond this 7c0ca58 should take care of your concerns about specifying a default value for the pulse to the mapping functions.

eldond commented 3 years ago

@orso82 Wow. If that works how I think it does, it's a very nice solution. I'm impressed. That's very clean. Keeps the argument right with the function, but controls when it's used.

eldond commented 3 years ago

@orso82 where's the automatic testing system?

[eldond@mag-198-129-105-101-gat-com omas]$ omas/tests/test_omas_physics.py 
Setting up OMAS warnings for user eldond
OMAS warnings set to hard mode
Warning:
  The MDSplus python module version (7.96.17) does not match
  the version of the installed MDSplus libraries (7.84.8).
  Upgrade the module using the mdsplus/python/MDSplus directory of the
  MDSplus installation or set PYTHONPATH=/usr/local/mdsplus/python/MDSplus.
Traceback (most recent call last):
  File "omas/tests/test_omas_physics.py", line 24, in <module>
    from omas.tests import warning_setup
  File "/home/eldond/omas/omas/tests/__init__.py", line 5, in <module>
    from test_omas_plot import *
  File "/home/eldond/omas/omas/tests/test_omas_plot.py", line 35, in <module>
    class TestOmasPlot(UnittestCaseOmas):
  File "/home/eldond/omas/omas/tests/test_omas_plot.py", line 41, in TestOmasPlot
    ods = ODS().sample()
  File "/home/eldond/omas/omas/omas_core.py", line 2214, in sample
    getattr(self, 'sample_' + func)()
  File "/home/eldond/omas/omas/omas_sample.py", line 361, in magnetics
    magnetics_hardware(ods)
  File "/home/eldond/omas/omas/omas_machine.py", line 576, in machine_mapping_caller
    out = f(*args, **kwargs)
  File "/home/eldond/omas/omas/machine_mappings/d3d.py", line 733, in magnetics_hardware
    mhdin.to_omas(ods, update='magnetics')
  File "/home/eldond/OMFIT-source/omfit/omfit_classes/sortedDict.py", line 776, in __getattr__
    return getattr(self, attr)
  File "/home/eldond/OMFIT-source/omfit/omfit_classes/sortedDict.py", line 780, in __getattr__
    raise AttributeError('bad attribute `%s`' % attr)
AttributeError: bad attribute `to_omas`
orso82 commented 3 years ago

@eldond you can fix that by using the latest omfit_classes

I have started to use some OMFIT classes in the machine mapping functions. What I'd like to do (at least at this point) is for the OMFIT classes dependency to be optional, which means that they should not be used to setup the sample ODS. I'll fix that. Also, this reminds me I still have to do a bit of work on the omas setup.py.

eldond commented 3 years ago

@orso82 Do you want to just merge this and make the gathering faster later? I don't know when I'm going to get to it.

orso82 commented 3 years ago

I understand and sounds good.