search

NCAS-CMS / cfunits

A Python interface to UNIDATA’s UDUNITS-2 library with CF extensions:
http://ncas-cms.github.io/cfunits
MIT License
11 stars 8 forks source link

Make platform independent #32

Closed daniel-mohr closed 3 years ago

daniel-mohr commented 3 years ago

At least most of #31 should be fixed with this pull request. For the given test cases the solution should be fine. Some problems addressed in #31 are not solved.

sadielbartholomew commented 3 years ago

@daniel-mohr FYI I have just pushed some commits I made yesterday and forgot to push upstream, one of which makes a change so that the linting job should now pass. I will review this tomorrow.

daniel-mohr commented 3 years ago

@sadielbartholomew I updated/merged your new master into my branch make_platform_independent. Unfortunately the tests do not run again and I also do not find a way to initiate a re-run. Any ideas? But the merge was simple.

sadielbartholomew commented 3 years ago

Thanks @daniel-mohr.

Unfortunately the tests do not run again and I also do not find a way to initiate a re-run. Any ideas?

With the current settings for the workflows, all (I think) having set:

https://github.com/NCAS-CMS/cfunits/blob/eca4441db8a124f4fac40b5011b359e40871c940/.github/workflows/run-test-suite.yml#L9-L11

the easiest way, though not particularly elegant, to re-trigger the jobs is to open and close the PR to hit the reopened category, which is what I have done for a while (feel free to try it and see). If you know a more elegant but equally simple way to re-trigger the jobs then please do let us know and/or update that part of the workflows to effect the necessary change in Actions behaviour.

daniel-mohr commented 3 years ago

@sadielbartholomew I commented your 'code comments', but after a new/better commit I cannot find them. But anyway, due to my better solution (based on my comments in #31) they are obsolete.

I will react on your other comments later (tomorrow or ...).

codecov-commenter commented 3 years ago

Codecov Report

Merging #32 (111b958) into master (eca4441) will increase coverage by 0.39%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
+ Coverage   80.71%   81.10%   +0.39%     
==========================================
  Files           2        2              
  Lines         679      677       -2     
==========================================
+ Hits          548      549       +1     
+ Misses        131      128       -3
Flag Coverage Δ
unittests 81.10% <100.00%> (+0.39%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cfunits/units.py 81.22% <100.00%> (+0.40%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update eca4441...111b958. Read the comment docs.

sadielbartholomew commented 3 years ago

Thanks @daniel-mohr, that sounds good. Take your time, there is unlikely to be any major or indeed minor changes going into cfunits across the next week, as far as I am aware, so there is little chance for significant merge conflicts etc.

daniel-mohr commented 3 years ago

Regarding the code: https://github.com/NCAS-CMS/cfunits/blob/eca4441db8a124f4fac40b5011b359e40871c940/cfunits/units.py#L2225-L2230

I'm also not glad about it. But I do not want to decide this. Let's wait for @davidhassell .

daniel-mohr commented 3 years ago

Adapting setup.py regarding windows: Should we only add "Operating System :: Microsoft :: Windows" or should we do "Operating System :: OS Independent"? (I think it is as independent as udunits is available -- it's python!)

sadielbartholomew commented 3 years ago

Adapting setup.py regarding windows: Should we only add "Operating System :: Microsoft :: Windows" or should we do "Operating System :: OS Independent"? (I think it is as independent as udunits is available -- it's python!)

Operating System :: OS Independent please, it is more concise than listing out all of the possibilities (Linux, MacOS and now Windows)!

sadielbartholomew commented 3 years ago

@daniel-mohr, one small thing to mention, FYI: I am quickly putting in a PR to do some automatic sorting of import statements across some libraries, including cfunits (#33), so there is the potential for a small merge conflict from that, though I see you aren't applying any imports at this point, so hopefully there won't be any at all.

daniel-mohr commented 3 years ago

'setup.py' is now adapted.

I have no intention to add further code to this PR. So, from my point of view a merge of this PR could be done.

sadielbartholomew commented 3 years ago

Thanks @daniel-mohr. The new commits look good to me, with the setup.py being adapted correctly etc.

Since @davidhassell is due back from leave tomorrow, and it would be good to have a second opinion on the changes here because the code touched in the units module is quite critical to cfunits working properly, I will do a final review later today (since you have changed approach) but wait to merge until David has signed this off too, likely in the next few days or so.

davidhassell commented 3 years ago

Hi @daniel-mohr and @sadielbartholomew - lot's has gone since I've been away - great. I'll have a look at the dtype issues today.

daniel-mohr commented 3 years ago

OK, if I can help you let me know!

By the way, I enhanced list_numpy_dtypes to show the type mismatches on i386 Linux platform.

davidhassell commented 3 years ago

Version 3.3.4 now available in PyPi. Thanks @daniel-mohr and @sadielbartholomew for your work on this. Much appreciated.