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

Fix: more general ovf reading #513

Merged
merged 4 commits into from
Nov 25, 2023
Merged

Fix: more general ovf reading #513

merged 4 commits into from
Nov 25, 2023

Conversation

lang-m
Copy link
Member

@lang-m lang-m commented Nov 24, 2023

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

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.

Closes ubermag/help#266
Copy link
Contributor

PR Analysis

  • 🎯 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

PR Feedback

  • 💡 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()

How to use

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.

@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2279d1e) 93.49% compared to head (52e427d) 93.49%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #513   +/-   ##
=======================================
  Coverage   93.49%   93.49%           
=======================================
  Files          28       28           
  Lines        3027     3027           
=======================================
  Hits         2830     2830           
  Misses        197      197           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lang-m
Copy link
Member Author

lang-m commented Nov 24, 2023

@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.

Copy link
Member

@swapneelap swapneelap left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍🏽

@lang-m lang-m merged commit 8b94305 into master Nov 25, 2023
7 checks passed
@lang-m lang-m deleted the fix-ovf-reading branch November 25, 2023 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decoding issue when transforming OVF to Field
4 participants