Closed lang-m closed 11 months ago
๐ฏ Main theme: This PR aims to make the ovf file reading process more flexible by ignoring the case of the data line in the ovf header.
๐ PR summary: The PR addresses an issue with the capitalisation of the data line in the ovf header, which is not well-defined in the standard. The PR makes the reading process more flexible by ignoring the case of the data line. This change is implemented in the ovf.py file.
๐ Type of PR: Bug fix
๐งช Relevant tests added: No
โฑ๏ธ Estimated effort to review [1-5]: 2, because the changes are straightforward and limited to a single file.
๐ Security concerns: No
๐ก General suggestions: The changes made in this PR are clear and straightforward. However, it would be beneficial to add tests that validate the new case-insensitive reading of the ovf header.
๐ค Code feedback:
relevant file: discretisedfield/io/ovf.py
suggestion: Consider using constants for repeated strings like "# begin: data" and "binary" to avoid potential typos in the future. [medium]
relevant line: if line.startswith("# begin: data"):
relevant file: discretisedfield/io/ovf.py
suggestion: It might be a good idea to handle potential exceptions that could be raised when decoding the line with utf-8. [medium]
relevant line: line = line.decode("utf-8").lower()
Instructions
To invoke the PR-Agent, add a comment using one of the following commands: /review: Request a review of your Pull Request. /describe: Update the PR title and description based on the contents of the PR. /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback. /ask \<QUESTION>: Ask a question about the PR. /update_changelog: Update the changelog based on the PR's contents. /add_docs: Generate docstring for new components introduced in the PR. /generate_labels: Generate labels for the PR based on the PR's contents. see the tools guide for more details.
To edit any configuration parameter from the configuration.toml, add --config_path=new_value. For example: /review --pr_reviewer.extra_instructions="focus on the file: ..." To list the possible configuration parameters, add a /config comment.
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
2279d1e
) 93.49% compared to head (52e427d
) 93.49%.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@samjrholt and @swapneelap I would like to merge this as soon as possible and release a small bug-fix I will add the file provided in the issue as additional test file if I get the permission.
Currently, we take capitalisation of the data line in the ovf header into account, however that does not seem to be well-defined in the standard. (We use the same format that OOMMF writes but the documentation on the OOMMF website uses a different standard.)
To make it more flexible, we can ignore the case.
Here are the specifications: https://math.nist.gov/oommf/doc/userguide21a0/userguide/Data_block.html
Closes ubermag/help#266