146 add magnetic suceptibility to ppms #154
Conversation
Separated schemas for ETO and ACT
Added the ACMS test
Reviewer's Guide by SourceryThis pull request implements support for ACMS (Susceptibility) measurements in the PPMS plugin. This involved adding a new schema for ACMS data, updating the PPMS measurement schema to accommodate ACMS measurements, and modifying the parser to handle ACMS data files. The changes also include adding functions to split and process ACMS data, as well as updating tests to cover the new functionality. Class diagram showing the new ACMS measurement structureclassDiagram
class PPMSMeasurement {
+normalize()
}
class PPMSACMSMeasurement {
+frequency_tolerance: float
+amplitude_tolerance: float
+normalize()
}
class ACMSPPMSData {
+time_stamp: float[]
+temperature: float[]
+magnetic_field: float[]
+frequency: float[]
+amplitude: float[]
+moment: float[]
+moment_derivative: float[]
+moment_second_derivative: float[]
+phase: float[]
}
class ACMSData {
+name: string
+map: float[]
}
PPMSMeasurement <|-- PPMSACMSMeasurement
PPMSACMSMeasurement o-- ACMSPPMSData
ACMSPPMSData o-- ACMSData
note for PPMSACMSMeasurement "New class for ACMS measurements"
note for ACMSPPMSData "Stores ACMS measurement data"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @JonathanNoky - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider refactoring the similar logic in split_ppms_data_act/eto/acms functions into a common utility function to reduce code duplication
- Add documentation describing the ACMS measurement parameters and data format
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if hasattr(eto_channel, 'name'): | ||
| setattr(eto_channel, 'name', key) | ||
| if hasattr(eto_channel, 'ETO_channel'): | ||
| setattr(eto_channel, 'ETO_channel', data[key]) |
There was a problem hiding this comment.
issue (bug_risk): Fix incorrect variable reference in ETO channel data assignment
This line uses 'data[key]' but should use 'block[key]' to access the data from the current block being processed.
docs/index.md
Outdated
| - **Physical Properies Measurement System (PPMS)**: Supports automatic parsing of | ||
| measurement files and sequence files from the Quantum Design PPMS. For now, ETO, ACT, | ||
| and ACMS modes are supported. Supported file formats: | ||
| - QuantumDesign PPMS: `*.dat ` and `*.seq` |
There was a problem hiding this comment.
issue (typo): Extra space in file extension
There's an extra space after ".dat". It should be *.dat.
| - QuantumDesign PPMS: `*.dat ` and `*.seq` | |
| - QuantumDesign PPMS: `*.dat` and `*.seq` |
| if line.startswith('FILEOPENTIME'): | ||
| if hasattr(self, 'datetime'): | ||
| try: | ||
| iso_date = datetime.strptime( | ||
| line.split(',')[3], '%m/%d/%Y %H:%M:%S' | ||
| ) # noqa: E501 | ||
| except ValueError: | ||
| try: | ||
| iso_date = datetime.strptime( | ||
| ' '.join(line.split(',')[2:4]), '%m-%d-%Y %I:%M %p' | ||
| ) # noqa: E501 | ||
| except ValueError: | ||
| iso_date = datetime.strptime( | ||
| line.split(',')[3], '%Y-%m-%d %H:%M:%S' | ||
| ) # noqa: E501 | ||
| setattr(self, 'datetime', iso_date) | ||
| iso_date = get_fileopentime(line) | ||
| if iso_date == 'Not found.': | ||
| logger.error( | ||
| 'FILEOPENTIME not understood. Check the format.' | ||
| ) | ||
| else: | ||
| setattr(self, 'datetime', iso_date) |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if line.startswith('FILEOPENTIME'): | |
| if hasattr(self, 'datetime'): | |
| try: | |
| iso_date = datetime.strptime( | |
| line.split(',')[3], '%m/%d/%Y %H:%M:%S' | |
| ) # noqa: E501 | |
| except ValueError: | |
| try: | |
| iso_date = datetime.strptime( | |
| ' '.join(line.split(',')[2:4]), '%m-%d-%Y %I:%M %p' | |
| ) # noqa: E501 | |
| except ValueError: | |
| iso_date = datetime.strptime( | |
| line.split(',')[3], '%Y-%m-%d %H:%M:%S' | |
| ) # noqa: E501 | |
| setattr(self, 'datetime', iso_date) | |
| iso_date = get_fileopentime(line) | |
| if iso_date == 'Not found.': | |
| logger.error( | |
| 'FILEOPENTIME not understood. Check the format.' | |
| ) | |
| else: | |
| setattr(self, 'datetime', iso_date) | |
| if line.startswith('FILEOPENTIME') and hasattr(self, 'datetime'): | |
| iso_date = get_fileopentime(line) | |
| if iso_date == 'Not found.': | |
| logger.error( | |
| 'FILEOPENTIME not understood. Check the format.' | |
| ) | |
| else: | |
| setattr(self, 'datetime', iso_date) | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
| if line.startswith('FIELDTOLERANCE'): | ||
| if hasattr(self, 'field_tolerance'): | ||
| setattr( | ||
| self, | ||
| 'field_tolerance', | ||
| float(line.replace('FIELDTOLERANCE,', '').strip()), | ||
| ) # noqa: E501 | ||
| ) |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if line.startswith('FIELDTOLERANCE'): | |
| if hasattr(self, 'field_tolerance'): | |
| setattr( | |
| self, | |
| 'field_tolerance', | |
| float(line.replace('FIELDTOLERANCE,', '').strip()), | |
| ) # noqa: E501 | |
| ) | |
| if line.startswith('FIELDTOLERANCE') and hasattr(self, 'field_tolerance'): | |
| setattr( | |
| self, | |
| 'field_tolerance', | |
| float(line.replace('FIELDTOLERANCE,', '').strip()), | |
| ) | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
tests/test_xrd.py
Outdated
| if len(parsed_measurement_archive.data.results[0].intensity.shape) == 1: | ||
| assert ( | ||
| parsed_measurement_archive.results.properties.structural.diffraction_pattern[ | ||
| 0 | ||
| ].incident_beam_wavelength.magnitude | ||
| * 1e10 | ||
| == pytest.approx(1.540598, 1e-2) | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| return all_steps, runs_list | ||
|
|
||
|
|
||
| def get_acms_ppms_steps_from_data_wip( # noqa: PLR0912, PLR0915 |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Move assignment closer to its usage within a block (
move-assign-in-block) - Simplify conditional into switch-like form [×2] (
switch) - Use f-string instead of string concatenation (
use-fstring-for-concatenation) - Low code quality found in get_acms_ppms_steps_from_data_wip - 8% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
| return all_steps, runs_list | ||
|
|
||
|
|
||
| def split_ppms_data_act(data_full, runs): # noqa: PLR0912 |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional [×3] (
use-named-expression) - Simplify conditional into switch-like form (
switch) - Don't assign to builtin variable
map(avoid-builtin-shadow) - Low code quality found in split_ppms_data_act - 21% (
low-code-quality)
Explanation
Python has a number of builtin variables: functions and constants that
form a part of the language, such as list, getattr, and type
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
list = [1, 2, 3]However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as integers.
In a pinch, my_list and similar names are colloquially-recognized
placeholders.
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
| return all_data | ||
|
|
||
|
|
||
| def split_ppms_data_eto(data_full, runs): # noqa: PLR0912 |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional [×3] (
use-named-expression) - Simplify conditional into switch-like form (
switch) - Low code quality found in split_ppms_data_eto - 20% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
| return all_data | ||
|
|
||
|
|
||
| def split_ppms_data_acms(data_full, runs): |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Simplify conditional into switch-like form (
switch) - Don't assign to builtin variable
map(avoid-builtin-shadow) - Low code quality found in split_ppms_data_acms - 20% (
low-code-quality)
Explanation
Python has a number of builtin variables: functions and constants that
form a part of the language, such as list, getattr, and type
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
list = [1, 2, 3]However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as integers.
In a pinch, my_list and similar names are colloquially-recognized
placeholders.
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
| @@ -180,258 +138,9 @@ class PPMSMeasurement(Measurement, PlotSection, EntryData): | |||
|
|
|||
| def normalize(self, archive, logger: BoundLogger) -> None: # noqa: PLR0912, PLR0915 | |||
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Replace m.group(x) with m[x] for re.Match objects (
use-getitem-for-re-match-groups) - Low code quality found in PPMSMeasurement.normalize - 11% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
More separate and reused functions
|
apparently, there are conflicts to solve in 4 files before merging |
|
Sorry, conflicts are solved now. |
PPMS plugin now supports ACMS (Susceptibility) measurements.
Summary by Sourcery
New Features: