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

IO AP sensing (Follow up of request #219 and #218) #220

Merged
merged 28 commits into from
Sep 12, 2024

Conversation

davechris
Copy link
Collaborator

@davechris davechris commented Aug 30, 2024

  • implemented generic .tra parser
  • added a test
  • added documentation to changelog
  • solved the linting errors in apsensing.py sensornet.py and sensortran.py

And I run the tests:

  • hatch run format --> successful
  • hatch run test --> does still not work, but the command < pytest ./tests/ > worked OK, I guess. I do not know what to expect, the result was: 80 passed, 3 skipped, 1 xfailed, 1 xpassed, 22 warnings in 580.50s (0:09:40)
  • hatch run docs:build --> build succeeded, 35 warnings -- I guess this is OK?

David Lah and others added 10 commits February 1, 2024 17:25
…0. C320 was an arbitrary name given for the wellbore by the user
… .xml and .tra exported, which gives more metadata, readings of the instruments temperature sensors, and instrument computed log-ratio and attenuation.
added a check wheather a .tra file with identical timestamp exists in .xml directory. If yes it is used to:
- import t_by_dts, log_ratio, loss
- pt100 data (if it exists in file)
@BSchilperoort
Copy link
Collaborator

BSchilperoort commented Aug 30, 2024

Good to see you've found some time to work on this again! I've invited you as a collaborator, so the CI should run without me needing to click the button.

It seems the linter complains now due to a change in its default config.

@davechris
Copy link
Collaborator Author

Yeah, nice to be back :) It took me some time, as I had a bunch of responsibilities last semester. It seems, that I get a lot of time for my research in the coming months.

Ah, that linter-config-change expains the complains with the printout commands.

Copy link
Collaborator

@BSchilperoort BSchilperoort left a comment

Choose a reason for hiding this comment

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

I had a look, didn't run the code yet (but I assumes that part is fine).

The CI is complaining, and you probably don't see those errors because your hatch environments use less new versions of packages. If you do hatch env prune you remove them and hatch will create new environments that should show the same errors as on the CI

tests/data/ap_sensing_2/readme_data_explanation.txt Outdated Show resolved Hide resolved
tests/test_datastore.py Outdated Show resolved Hide resolved
CHANGELOG.rst Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
src/dtscalibration/io/apsensing.py Outdated Show resolved Hide resolved
src/dtscalibration/io/apsensing.py Outdated Show resolved Hide resolved
src/dtscalibration/io/apsensing.py Show resolved Hide resolved
src/dtscalibration/io/apsensing.py Outdated Show resolved Hide resolved
src/dtscalibration/io/apsensing.py Outdated Show resolved Hide resolved
davechris and others added 12 commits September 9, 2024 13:24
Co-authored-by: Bart Schilperoort <b.schilperoort@gmail.com>
Co-authored-by: Bart Schilperoort <b.schilperoort@gmail.com>
Co-authored-by: Bart Schilperoort <b.schilperoort@gmail.com>
Co-authored-by: Bart Schilperoort <b.schilperoort@gmail.com>
- changed detection algorithm of .tra files
- added a double-ended calibration test of ap_sensing_2 data to test_datastore.py
    - load_tra_arrays flag
    - current .tra implementation is limited to in-memory reading only
@davechris
Copy link
Collaborator Author

davechris commented Sep 9, 2024

The CI is complaining, and you probably don't see those errors because your hatch environments use less new versions of packages. If you do hatch env prune you remove them and hatch will create new environments that should show the same errors as on the CI

hatch env prune worked. So it seems that I can finally execute all hatch commands!

I started with hatch run format:
...\dtscalibration\lib\site-packages\bokeh\resources.py:363: error: Pattern matching is only supported in Python 3.10 and greater [syntax]
Found 1 error in 1 file (errors prevented further checking)

Either we need to change that pattern matching or require Python 3.10 in pyproject.toml:

  • line 26 --> requires python 3.10 (from 3.9)
  • line 131 delete "3.9" from array
  • line 173 target version py310 statt py39

then it continues to search for errors, finds 10, fixes 8. The two remaining are identical:
UP038 in assert isinstance(v, (list, tuple)) in src\dtscalibration\dts_accessor_utils.py
Very easy to fix (https://docs.astral.sh/ruff/rules/non-pep604-isinstance/)

The question is: Do you want to require python 3.10 or rather fix the first errror? - Can we tell the linter to ignore site packages?

@BSchilperoort
Copy link
Collaborator

BSchilperoort commented Sep 10, 2024

...\dtscalibration\lib\site-packages\bokeh\resources.py:363: error: Pattern matching is only supported in Python 3.10 and greater [syntax] Found 1 error in 1 file (errors prevented further checking)

Oh that's an annoying bug. We probably have to run the CI at Python 3.9 to make it green. Bokeh has dropped support for 3.9. We don't use it, but it's being installed due to xarray[parallel] being in the dependencies, which then installs dask[complete] which needs bokeh.

Let's just ignore this error for now, that's less work 😉 It won't break any code anyway.

Can we tell the linter to ignore site packages

Not sure why it's checking it, I don't think it should.

I think the only thing to fix still is the docs, the build seems to fail. That's more important that the small mypy error. Do you think you can fix the notebooks that are failing? You should be able to build the docs locally with hatch run docs:build.

@BSchilperoort
Copy link
Collaborator

You can try changing the settings:

[tool.mypy]
ignore_missing_imports = true  # Preferably false, but matplotlib, scipy and statsmodels are missing typing stubs
python_version = "3.9"
exclude="*/site-packages/*"

@davechris
Copy link
Collaborator Author

Alright, I'll try that tomorrow.

Copy link
Collaborator

@BSchilperoort BSchilperoort left a comment

Choose a reason for hiding this comment

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

CI is now green. If you can have a look at the delayed loading (into dask arrays) #220 (comment) and see if that still works or not (and add that info to docs), you can merge this PR.

@davechris
Copy link
Collaborator Author

I added a remark to document that issue in the docstring and in the docs notebook A3.

And I do not have the rights to merge a PR ;)

@BSchilperoort BSchilperoort merged commit 4a3aa8a into dtscalibration:main Sep 12, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants