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

MAD-NG Features and Compatibility Modes #135

Draft
wants to merge 265 commits into
base: master
Choose a base branch
from
Draft

MAD-NG Features and Compatibility Modes #135

wants to merge 265 commits into from

Conversation

fsoubelet
Copy link
Member

@fsoubelet fsoubelet commented Oct 15, 2024

MAD-NG Compatibility

This PR close #104 and closes #105. I will make a new issue for some exotic caveats (see caveat subsection below).

While users relying on default parameters will not be affected, I label this as a major version release as there is a substantial amount of behavioural changes.

Main Changes

Full details relevant to the users can be found in the Changelog, details are given in sections below. Main items are:

  • Handling of boolean and complex headers values (MAD-NG feature).
  • Handling of bolean-type and complex-type columns (MAD-NG feature).
  • Compatibility modes for dataframe validation (details below).

Some changes and fixes also show up:

  • TfsDataFrame validation is now skipped on reading by default.
  • TfsDataFrame validation is by default done in MAD-X compatibility mode before writing.
  • Writing a dataframe with no headers (not empty headers) - e.g. a pandas.DataFrame - now works correctly.

MAD-NG Features

Changes have been made to support boolean and complex values, both in headers and columns.
Special care is taken for the parsing of complex values given that the MAD-NG representation uses i for the complex part while Python uses j.

I am adding comments on the PR to help review.

Caveat

Some of the more exotic features that can happen from MAD-NG are not supported, as discussed. These include for instance the overloaded ranges (i.e. 1..7) and for these the user should use pymadng and do conversions in memory. I will create an issue on the repo to be tagged "wontfix" or similar.

Should a user encounter this, then an UnknownTypeIdentifierError is raised (see maintenance changes). I am open to directing them towards pymadng in the error message.

Validation

The validation has been reworked and offers compatibility guarantees with MAD-X or MAD-NG. A valid mode should be provided to the tfs.frame.validate function, which accepts MADX / MAD-X / MADNG/MAD-NG` (case-insensitive).

Validation can still be skipped when reading or writing by providing None.

Details on the applied rules are given in the documentation so I won't duplicate them here. I'll point it out as PR comments.

Added Tests

I have added an entirely new module for all tests related to validation.
I have also added a bunch of reader and writer tests to cove the new (MAD-NG) features.
I have added new TFS files in the inputs for each of these features, and I have added a compressed version (for each supported format) of a file containing them all to the relevant folder.

Added Documentation

Docstrings have been updated everywhere relevant.

The documentation now holds a dedicated page detailing the TFS format (this closes #105).
There is also a dedicated page about validation and compatibility modes (this closes #104).

I heavily encourage downloading the built documentation from CI and having a look locally.

Maintenance Changes

The most substantial maintenance change is the introduction of many new errors to be raised, which are each specific to a given problem one might encounter. They all inherit from the (previously unique) TfsFormatError, so users that were catching this one will not see their code broken.

I have made a slew of maintenance changes to the documentation: fixing warnings during generation, updating deprecated configuration parameters, fixed typos, tried to make a lot of things more explicitely explained, fixed broken links etc.

There are also a bunch of maintenance changes made to the tests. Mainly, I have moved quite a few common fixtures to the conftest.py file to avoid the duplications we had through files. I have tried to separate a bit more different testing parts.

I will add PR comments to the code to make it easy to determine what is new vs what is maintenance.

@fsoubelet
Copy link
Member Author

After consulting with Laurent:

  • The expected length identifier for string headers was there for super old reasons and MAD-NG does not actually care for it. The check will be removed (and replaced by a check that an actual string dtype was provided as value) in the next MAD-NG release.
  • The invalid %d identifier for integers was an oversight and will be handled in the next MAD-NG release.

In case of issues with users we can simply direct them to update their MAD-NG/pymadng.

I will still add some pymadng tests but we will have to wait for a new release of both MAD-NG and pymadng for these to work as expected, which pushes back the ETA of our release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High Work on this! Status: Review Needed Work currently stopped, untils someone else reviews it. Type: Documentation Improvements, updates and fixes to the documentation. Type: Feature A (suggetion for a) new feature or enhancement in functionality. Type: Maintenance Improvements in the code, that are not necessarily visible in functionality. Type: Release Issue preparing for a release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: TFS definition [Feature Request]: Check for madx-compatibility
3 participants