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

added boolean %b to headers for MADNG output #124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fscarlier
Copy link

Added boolean %b to headers since MADNG is using it in its twiss files. Added test in reader as well. Only for headers at the moment, since there is no usecase yet for boolean columns

@fsoubelet
Copy link
Member

I'm away and can't check right now but what happens when this is given to MAD-X?

One of the important points of tfs-pandas is that whatever MAD-X gives it can read, and whatever it writes MAD-X can read. This is the reason that, for now, no features of MAD-NG have made it in (such as the complex-type columns).

I imagine this would be a problem for MAD-X, so I think this warrants careful discussion with developers and consulting of our user-base before we commit to what could be a significant change. A big change could also be the opportunity to include other MAD-NG-specific features.

If my suspicion above is true, then at the very least I recommend that you add in this PR a warning logged when writing out a TfsDataFrame that includes a boolean header, letting the user know clearly that this is going to make MAD-X complain / crash. If not, then it looks pretty good as it is! Let me know :)

@fsoubelet fsoubelet added Enhancement New feature or request To be discussed Decision or implementation depends on external factors and requires discussion within the OMC team Feature Request labels Jul 26, 2023
Comment on lines +142 to +147
assert df.headers['BOOLTRUE1'] == True
assert df.headers['BOOLTRUE2'] == True
assert df.headers['BOOLTRUE3'] == True
assert df.headers['BOOLFALSE1'] == False
assert df.headers['BOOLFALSE2'] == False
assert df.headers['BOOLFALSE3'] == False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should be is True and is False.

@JoschD
Copy link
Member

JoschD commented Jul 26, 2023

Regarding @fsoubelet 's comment: I think MAD-NG is actually coming, so we need to adapt to it. Didn't we discuss something like a switch "compatibility="MAD-X" in the write function?

@fscarlier
Copy link
Author

fscarlier commented Jul 26, 2023

I agree with @JoschD. Madng is slowly being used and tested, and it will probably replace PTC for normal form/rdt use cases quite soon. Small packages like tfs-pandas should be adapted to help any transition. The "madx" switch could be a good option indeed.

Also, I'm not a fan of a warning. Looks like pythonic madx..

@fsoubelet
Copy link
Member

fsoubelet commented Jul 27, 2023

Then this could be the opportunity for a few PRs before the next release (taking care of #104 in the same go for instance). I like a switch that would default to MAD-X for now and make the user aware.

Also, I'm not a fan of a warning. Looks like pythonic madx..

We can log and raise an error if we feel like this is the best option, I was merely suggesting ;)

@JoschD
Copy link
Member

JoschD commented Jul 28, 2023

I would just adapt the "validate" function and the validate-switch, such that you can turn it on/off as before but also give it as "MAD-NG" or "MAD-X" and it checks for different things. Would be a minor change in that regard.

The big change would be to always convert to the new datatypes %b for boolean and %i for complex. If the user wants MAD-X compatibility they need to change the dtype of the respective columns/headers or split the value in real/imag manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request Feature Request To be discussed Decision or implementation depends on external factors and requires discussion within the OMC team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants