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

chore: Update dependency to AFMReader #852

Merged
merged 6 commits into from
Jun 6, 2024
Merged

chore: Update dependency to AFMReader #852

merged 6 commits into from
Jun 6, 2024

Conversation

ns-rse
Copy link
Collaborator

@ns-rse ns-rse commented Jun 6, 2024

Closes #851

With renaming topofileformats > AFMReader and removing the former from PyPI we had to update the dependency.

As detailed in #851 the sum of the arrays changes on switching from topofileformats > AFMReader. The issue notes some details of investigation.

Also took the opportunity to update the installation instructions to make it clearer and include information on installing from GitHub using pip.

With renaming `topofileformats` > `AFMReader` and removing the former from PyPI we had to update the dependency.

Currently two tests fail...

```
FAILED tests/test_io.py::test_load_scan_asd - assert -1368044348.3393068 == -71724923530211.84
FAILED tests/test_io.py::test_load_scan_get_data[load_scan_asd-197-image_shape5--673381139990.2344-file_122-2.0] -
assert -12843725.967220962 == -673381139990.2344
```
@ns-rse ns-rse requested a review from SylviaWhittle June 6, 2024 08:53
As detailed in #851 the sum of the arrays changes on switching from `topofileformats` > `AFMReader`. The issue notes
some details of investigation.
@MaxGamill-Sheffield
Copy link
Collaborator

Looks good and clean - Thanks for the update to the docs too Neil

Seems to work for the asd file we've been given, and the classic minicircles spm. A few comments though:

  1. I think it'd be useful if the available channels were returned upon failure, these are printed in the logger under debug and so I think it shouldn't be too bad to implement (I can do this if necessary)
  2. The logger exceptions for unclosed log files are throwing messy lines:
(asdtest) Max:TopoStats Maxgamill$ topostats process -c test/default_config.yaml 
Exception ignored in: <_io.FileIO name='/Users/Maxgamill/Desktop/Uni/PhD/topo_asd/TopoStats/TopoStats-2024-06-06-10-28-12.log' mode='ab' closefd=True>
Traceback (most recent call last):
  File "/Users/Maxgamill/Desktop/Uni/PhD/topo_asd/TopoStats/topostats/statistics.py", line 8, in <module>
    LOGGER = setup_logger(LOGGER_NAME)
ResourceWarning: unclosed file <_io.TextIOWrapper name='/Users/Maxgamill/Desktop/Uni/PhD/topo_asd/TopoStats/TopoStats-2024-06-06-10-28-12.log' mode='a' encoding='UTF-8'>
Exception ignored in: <_io.FileIO name='/Users/Maxgamill/Desktop/Uni/PhD/topo_asd/TopoStats/TopoStats-2024-06-06-10-28-12.log' mode='ab' closefd=True>
Traceback (most recent call last):
  File "/Users/Maxgamill/Desktop/Uni/PhD/topo_asd/TopoStats/topostats/processing.py", line 31, in <module>
    LOGGER = setup_logger(LOGGER_NAME)
ResourceWarning: unclosed file <_io.TextIOWrapper name='/Users/Maxgamill/Desktop/Uni/PhD/topo_asd/TopoStats/TopoStats-2024-06-06-10-28-12.log' mode='a' encoding='UTF-8'>
[Thu, 06 Jun 2024 10:28:12] [INFO    ] [topostats] The YAML configuration file is valid.
[Thu, 06 Jun 2024 10:28:12] [INFO    ] [topostats] The YAML plotting configuration file is valid.

@ns-rse
Copy link
Collaborator Author

ns-rse commented Jun 6, 2024

Thanks for having a look @MaxGamill-Sheffield

I'm going to avoid scope creep and keep this PR focused on switching topofileformats > AFMReader (despite cramming in changes to the docs/installation.md). Please create issues to keep track of these suggestions and they can be picked up and addressed as and when (I basically don't have time to address them right now).

Copy link
Collaborator

@MaxGamill-Sheffield MaxGamill-Sheffield 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

@ns-rse ns-rse disabled auto-merge June 6, 2024 14:07
@ns-rse ns-rse merged commit b40239f into main Jun 6, 2024
14 checks passed
@ns-rse ns-rse deleted the ns-rse/851-afmreader branch June 6, 2024 14:07
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.

[Bug]: Update topofileformats dependency to AFMReader [URGENT]
2 participants