cherab / solps

Other
6 stars 5 forks source link

Error reading fort.44 file from recent SOLPS #15

Closed jacklovell closed 4 years ago

jacklovell commented 4 years ago

The fort.44 loading function errors out when reading files produced by a recent version of SOLPS-ITER. The format identifier for these files is 20170328.

The Eirene reading logic has been tested on a file with format ID 20130210 (AUG test case), and I've also successfully read an Eirene file with format ID 20070912. But 20000727 does not work.

We should explicitly check the Eirene format ID, and raise an error for unsupported versions until new routines to read these format IDs are added.

Mateasek commented 4 years ago

I agree, last week I tried to read file with format ID 20170328 (first time I had to work with this module) which has comments before data blocks. This is not expected by the parsing script.

I think it would be in general a better approach to pull the file parsing code outside of Eirene class and make it accept simple np arrays as it is in the case of the EFITEquilibrium class. Then the parsing could be done in methods which can either return data in a dictionary or instance of the Eirene class directly. This way there will be simple way how to add parsing for more versions.

jacklovell commented 4 years ago

I have an in-progress branch which is able to read the 20170328 format of this. Like @Mateasek suggests, I've split the 2013 and 2017 formats into separate functions, with a top level function whose only job is to read the format ID and delegate to the appropriate function. I'll clean this up and submit it as a PR for review.

Mateasek commented 4 years ago

Awesome, thanks @jacklovell. Tell me if you need any help.

Mateasek commented 4 years ago

I tested the funciton reading 2017 fort.44 file version in the #16. I anyway think that the parsing should be moved outside of the eirene class, which would become only a data containter with defined inputs, variables and properties. There are two main reasons.

First is it will give the possibility to move the file reading code into separate files which will make the maintenance easier in my opinion. Managing of functions and assigning eirene versions in case a function covers mutiple would be easy and a new function or class can be added any time a new incompatible eirene version comes up.

Second is that this solps package in my opinion should handle only data formats exported by solps and other formats specific to institutes or machies should be handled externally. They could provide neccessary data in a specified format to the solps classes from this repository like it is in the case of EFITEquilibrium class. This would in my opinion also make the maintenance easier. An example is that for my case I would create a "solps data getter" class for COMPASS in the compass repository using classes from this repository.

jacklovell commented 4 years ago

I agree that moving the parsing out of the class would allow for easier expansion in future to other Eirene fort.44 formats. This sort of refactor is perhaps better done as part of the more extensive refactoring proposed in #8. I'll merge the fix in #16 as it provides a solution for an immediate need, with the understanding that it can be improved upon later.

jacklovell commented 4 years ago

Fixed with #16.