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

non-optional field /measurement/isSparsityTransformed missing when generating mdf #34

Open
hofmannmartin opened this issue Apr 23, 2021 · 6 comments

Comments

@hofmannmartin
Copy link
Member

This field should be part of every mdf we write, since it is non-optional. Currently, it is not.

@tknopp
Copy link
Member

tknopp commented Apr 23, 2021

indeed. can be fixed by using the approach taken here:

write(file, "/acquisition/numPeriods", get(params,:acqNumPeriodsPerFrame,1))

so just use the get function

@tknopp
Copy link
Member

tknopp commented Apr 23, 2021

if its not in the dict, then the data is not transformed.

@jonschumacher jonschumacher mentioned this issue Apr 23, 2021
4 tasks
@jonschumacher
Copy link
Member

jonschumacher commented Apr 26, 2021

There are more issues like this. E.g this line

if hasKeyAndValue(params, :measData)

requires :measData to be set in order to set the non-optional fields isFourierTransformed etc. The same applies to :measIsBGFrame.

I found this while working in #35 but I am not sure, if changing it creates errors downstream, so I will leave it alone and implement a direct version for converting between in-memory and file-based MDFs.

@tknopp
Copy link
Member

tknopp commented Apr 27, 2021

I don't fully get what you mean. The field :isFourierTransformed is only non-optional if measurements are actually stored, so if there is no :measData then the entire group will not be available.

You are probably right that I did not think through all edge cases in this code but it is rather pragmatic (and not ideal) but in this particular case I am not sure if that is actually wrong. But in general I absolutely agree that we should go with a more structural way and

  • either remove the dicts altogether.
  • just use them at the edge and convert it to the in-memory version prior to conversion.
    So in short, I absolutely support what is happening here.

@jonschumacher
Copy link
Member

True, I missed that part. Sorry for that!
Concerning the dict: Do you think it makes sense to just implement getindex and setindex! for the in-memory MDF and thereby mimic the dict behaviour while having the full consistency checking? Non-standard keys could simply go to the custom field of the in-memory MDF.

@tknopp
Copy link
Member

tknopp commented Apr 27, 2021

Not sure about implementing getindex and setindex! feels not that Julian to me. I would rather go in the direction of a conversion as we have done in MPIMeasurements.jl

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

No branches or pull requests

3 participants