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

Capitalize plane keys in Tfs class #47

Closed
lmalina opened this issue Sep 9, 2020 · 2 comments · Fixed by #48
Closed

Capitalize plane keys in Tfs class #47

lmalina opened this issue Sep 9, 2020 · 2 comments · Fixed by #48
Assignees
Labels
Type: Feature A (suggetion for a) new feature or enhancement in functionality.
Milestone

Comments

@lmalina
Copy link
Member

lmalina commented Sep 9, 2020

To improve consistency among our packages capitalize the plane keys for "planed" data frames, i.e. ("X", "Y") instead of ("x", "y").
Requires change of _define_property_two_planes(args, kwargs).

@lmalina lmalina added the Type: Feature A (suggetion for a) new feature or enhancement in functionality. label Sep 9, 2020
fsoubelet added a commit that referenced this issue Sep 16, 2020
- capitalize plane letter in file naming for consistency accross pylhc packages
- change hardcoded test input file names according to the new behavior
- change hardcoded test expected results according to the new behavior
- bump tfs-pandas version as 2.0.0 as discussed today
fsoubelet added a commit that referenced this issue Sep 16, 2020
- Bump version to 2.0.0
- Update dependency to python >= 3.7 for pylhc packages consistency (see acc-py stack)
- Removed unused test dependencies
- Removed setup_requires as it is deprecated
- Reworked setup.py to be consistent with that of pylhc/omc3
- Some formatting
@fsoubelet fsoubelet added this to the 2.0.0 milestone Sep 16, 2020
@fsoubelet fsoubelet mentioned this issue Sep 16, 2020
12 tasks
@fsoubelet fsoubelet linked a pull request Sep 16, 2020 that will close this issue
12 tasks
fsoubelet added a commit that referenced this issue Sep 16, 2020
…fsDataFrame repr and avoid shadowing built-in "id" in _dtype_to_str
fsoubelet added a commit that referenced this issue Sep 18, 2020
Worst case scenario, if pandas decides to change this private api then only our tests will suffer but not the package in itself
@JoschD
Copy link
Member

JoschD commented Sep 21, 2020

Not in the file, names, right?

@lmalina

@fsoubelet
Copy link
Member

fsoubelet commented Sep 21, 2020

Small write-up here so that it's always clear:

When a TfsCollection includes two planes, the attributes are appended with the plane keys (so we get, say, BET_x).

Previously the collection module implements this with a lowercase plane key (as in the example above). This issue is about respecting the pylhc convention of having this key capitalized (above example becomes BET_X).

The chosen implementation (see commit 37844ba later on corrected in dfbc675) uses .lower() in the __getitem__ and __setitem__ methods of the _TwoPlanes class. This way, internally the attributes are stored with a lowercase key, but when accessing them both lowercase and capitalized keys are accepted.

This is behavior is now also tested in test_collection.py.

Not in the file, names, right?

This was my mistake.
Convention for the file names across pylhc is still to have the plane key as lowercase.

@JoschD @lmalina please confirm this is correct.

fsoubelet added a commit that referenced this issue Sep 23, 2020
fsoubelet added a commit that referenced this issue Sep 23, 2020
…gers are found, in hopes that it teaches windows not to default to a C-compatible 32bits integer

Additional:
 - Include comment lines in tfs test files to ensure testing their detection (and ignoring them)
 - Add logging.error statements before raising exceptions to leave traces in log files if traceback view is not an option (htcondor etc)
fsoubelet added a commit that referenced this issue Sep 23, 2020
JoschD added a commit that referenced this issue Sep 23, 2020
JoschD added a commit that referenced this issue Sep 23, 2020
fsoubelet added a commit that referenced this issue Sep 24, 2020
added tests for coverage and raises (incl. test inputs)
fsoubelet added a commit that referenced this issue Sep 24, 2020
fsoubelet added a commit that referenced this issue Sep 24, 2020
…ferring the types conversion to an outer version

Additional:
- rename tools' logger to LOGGER for consistency
- some formatting
fsoubelet added a commit that referenced this issue Sep 24, 2020
As agreed verbally:
- notify in changelog that fixedtfs has been remove
- upgrade pytest-cov dependency to >=2.9, which explicitely guarantees python 3.8 compatibility on PyPI
- test coverage for a not implemented method in TfsCollection

We have checked that omc3 tests run fine with tfs 2.0.0 (code from this PR) :)
fsoubelet added a commit that referenced this issue Sep 24, 2020
- include objects non-loading in changelog
- remove commented out lines in doc/conf.py
- type hint and docstring in handler
fsoubelet added a commit that referenced this issue Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature A (suggetion for a) new feature or enhancement in functionality.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants