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

WIP: Julia 0.7, remove globals, missing value, scale_offset #61

Closed
wants to merge 7 commits into from

Conversation

meggart
Copy link
Member

@meggart meggart commented Jul 3, 2018

This is work in progress that I started some time ago and unfortunately is not based on the last updates (eg. #59 , #55 etc.) This basically happened when I was working on the code anyway to do the julia 0.7 update and includes the following new features:

  • runs on julia 0.7 without deprecation warnings
  • should be more thread-safe by removing the pre-allocated global arrays and using MArrays and SArrays where appropriate, there
  • also does not keep track of opened files anymore in global state, the high-level functions (ncread, ncwrite etc) all close the file after the operation is finished. The NetCDF.open function gets a do syntax to safely access files with the intermediate and array-like approach (see unit tests). This should make accessing the files more stable
  • this is probably a merge/review nightmare, but I splitted up NetCDF.jl in several files to number of LOC per file to a reasonable limit
  • add the replace_missing option to ncread and NetCDF.open, so that the new Array{T,Missing} can be read and written to/from files, this is done without making a copy of the data
  • respect the scale_factor and add_offset attributes and apply at least readvar in-place. The way to make this work without changing the data types is by letting the NetCDF library do the conversion

I think this would fix the main concerns that were raised in #39

As I said, when I started this I did not foresee so much activity by other community members (thanks to @jmgnve ). I think one could either apply the last PRs #54 #57 #59 on top of this one. The problem is that I will offline for a few weeks and can only continue here end of July.

The other possibility would be to simply merge #59 and also do what @visr siggested JuliaGeo/meta#4 run nanosoldier to have a 0.7-compatible version as fast as possible. Then I would have to apply the changes proposed above. Let me know what you think, and if I don't reply, fell free to merge the other pull requests as needed.

@visr
Copy link
Member

visr commented Jul 4, 2018

Ha nice! This is indeed pretty extensive :)

How about we merge #59 to master first, tag another 0.6 release, then rebase this branch on top of it.

The other open PR, #60 is code formatting. This will be more work to rebase on, so better we just apply it again after this PR is in. It was done automatically right, @jmgnve? Which tool did you use?

Currently the REQUIRE is set to 0.7. That unfortunately does not exist yet :). We can set it to 0.7-beta. Then femtocleaner will run as well, possibly catching anything you might have missed.

@jannefiluren
Copy link
Contributor

Hi! Yes, that is done automatically using the Julia extension to VS code. I can easily do that on top of the new changes if you like. It takes a couple of minutes. So happily ignore #60 meanwhile.

I really like your work on this pkg. It is a very important one. Many thanks.

@meggart
Copy link
Member Author

meggart commented Aug 23, 2018

I think given the last changes I will not continue on this PR but will apply the list of changes described above in multiple smaller PRs. Will leave this one open anyway until there is a new PR.

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.

3 participants