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

ncks append mode fix and travis improvements #29

Merged
merged 8 commits into from
Jun 15, 2017
Merged

Conversation

kwilcox
Copy link
Member

@kwilcox kwilcox commented May 22, 2017

No description provided.

@kwilcox
Copy link
Member Author

kwilcox commented May 23, 2017

I added Travis cleanup to this PR. As you can see (when the matrix finishes). It isn't reasonable to test all versions of nco so I picked a few "easy to get" versions:

  • 4.0.8 (apt-get on precise) - failing
  • 4.5.5 (conda-forge) - passing
  • 4.6.1 (conda-forge - selfishly added - it is what I use in production) - passing
  • 4.6.6 (conda-forge) - failing

IMO pynco shouldn't be writing conditionals to support all versions of nco (ie. warn when flags have been deprecated) and we should leave it up to the user to use the correct version for their needs. We should re-write the tests to be generic enough to pass on most versions of nco and add new nco releases to the travis matrix as needed. Thoughts?

@kwilcox kwilcox changed the title ncks append mode fix and a small fix for travis tests ncks append mode fix and travis improvements May 23, 2017
@czender
Copy link
Member

czender commented May 23, 2017

First, many thanks for making the CI tests more robust and fixing bugs. Personally I want the latest conda build of NCO the target of the scant developer attention to PyNCO. Not sure why 4.6.6 currently fails. Am about to release 4.6.7 of NCO, please let me know if there is anything I can do upstream that would turn 4.6.7 into a pass.

@ocefpaf
Copy link

ocefpaf commented May 23, 2017

@kwilcox do you need nco 4.0.8? We can add that version of conda-forge and simplify travis-ci here if you want.

@kwilcox
Copy link
Member Author

kwilcox commented May 23, 2017

@ocefpaf Yes, that is a good plan if we do indeed want to test 4.0.8. I don't use anything but 4.6.1 but I thought it would be good to test the version of nco that is distributed through debian/ubuntu. I would be OK with only testing the latest releases of all minor versions that are available in conda: 4.5.5 and 4.6.6.

@kwilcox
Copy link
Member Author

kwilcox commented May 23, 2017

@czender If I use 4.6.6 to change the _FillValue attribute (using ncatted) the entire variable is set to the _FillValue instead of only the values which match the value I specify. I don't know if you know about this or if its documented, but that is the major blocker for me updating. If you need a simple file and command to run to reproduce I can whip one up.

@czender
Copy link
Member

czender commented May 23, 2017

@kwilcox please provide an example. this output shows the requested change from 1.0e36 to -1.0e36 occurs only in missing values:

zender@firn:~$ ncks --cdl -C -v mss_val ~/nco/data/in.nc
netcdf in {
  dimensions:
    lon = 4 ;

  variables:
    float mss_val(lon) ;
      mss_val:long_name = "partial missing value example" ;
      mss_val:_FillValue = 1.e+36f ;

  data:
    mss_val = 73, _, 73, _ ; 

} // group /
zender@firn:~$ ncatted -O -a _FillValue,mss_val,m,f,-1.0e36 ~/nco/data/in.nc ~/foo.nc
zender@firn:~$ ncks --cdl -C -v mss_val ~/foo.nc
netcdf foo {
  dimensions:
    lon = 4 ;

  variables:
    float mss_val(lon) ;
      mss_val:long_name = "partial missing value example" ;
      mss_val:_FillValue = -1.e+36f ;

  data:
    mss_val = 73, _, 73, _ ; 

} // group /

@kwilcox
Copy link
Member Author

kwilcox commented May 23, 2017

@czender I'll elaborate here: nco/nco#53

@czender
Copy link
Member

czender commented May 24, 2017

@kwilcox i fixed nco/nco#53

.travis.yml Outdated
- 3.6

env:
- NCO_VERSION=apt
Copy link

Choose a reason for hiding this comment

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

@kwilcox b/c of #29 (comment) I tried to add the old 4.0.8 version to conda-forge.

I could not get a tarball from sourceforge nor GitHub, so I used the commit hash instead. But that version is too old I was unable to get it to compile. Maybe we should drop it from the matrix here. (And someone should update the debian package!)

my build attempt: https://github.com/ocefpaf/nco-feedstock/tree/nco_4.0.8
the CI results: https://circleci.com/gh/ocefpaf/nco-feedstock/tree/nco_4.0.8

@czender
Copy link
Member

czender commented May 31, 2017

@ocefpaf @kwilcox old (pre-GitHub) NCO tarballs are available from SourceForge, e.g.,

http://nco.sourceforge.net/src/nco-4.0.8.tar.gz

@ocefpaf
Copy link

ocefpaf commented May 31, 2017

Thanks @czender but the difficulty to build still remains. I don't think it is worth supporting this old version.

@kwilcox
Copy link
Member Author

kwilcox commented Jun 7, 2017

@czender Did you see Unidata/netcdf-c#390 and Unidata/netcdf-c#387?

edit: sorry, wrong issue. 🧠 :mush:

@kwilcox
Copy link
Member Author

kwilcox commented Jun 7, 2017

This is ready to go!

@kwilcox
Copy link
Member Author

kwilcox commented Jun 8, 2017

I'd like to get this merged and a release cut so I can get the conda-forge package updated. Trying real hard not to build my own conda packages lately!

@czender
Copy link
Member

czender commented Jun 8, 2017

@kwilcox Thanks so much for this PR. I prefer @jhamman to review/merge pynco PRs. Joe?

@czender czender merged commit 32b3f1a into nco:master Jun 15, 2017
@czender
Copy link
Member

czender commented Jun 15, 2017

Merged this since it passes all tests and have not heard from @jhamman. If things blow-up we can revert it. Thanks again for the patches, @kwilcox.

@jhamman
Copy link
Member

jhamman commented Jun 20, 2017

Thanks @kwilcox for the fix. I'm still digging out from a vacation, hence the lack of review/reply on this. Great that you were able to get the travis tests running again. That closes bf0aae5!

@jhamman jhamman mentioned this pull request Jun 20, 2017
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.

4 participants