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

Julia example #1240

Merged
merged 15 commits into from
Dec 19, 2019
Merged

Julia example #1240

merged 15 commits into from
Dec 19, 2019

Conversation

jhardenberg
Copy link
Contributor

This PR implements an example diagnostic in pure Julia which was missing and which can serve as a base for further Julia diagnostics. Provenance logging is included (since no library provides this I had to write a small YAML writer in diag_scripts/shared/external.jl). Resolves #1238

@valeriupredoi
Copy link
Contributor

please merge the latest version2_development into your branch to solve the failed test(s)

@bouweandela
Copy link
Member

Great contribution, this will make Julia support more visible!

since no library provides this I had to write a small YAML writer in diag_scripts/shared/external.jl

I think you can use the json library to write the provenance, since json is a subset of yaml.

@bouweandela
Copy link
Member

Regarding the code style, it would be nice if you could use more functions, even if it's just creating a main function and then running that.

@jhardenberg
Copy link
Contributor Author

I think you can use the json library to write the provenance, since json is a subset of yaml.

Thanks for the suggestion @bouweandela , indeed I considered it at the beginning but then I chose another approach. I now changed to code to use JSON.jl and indeed it works fine.

@jhardenberg
Copy link
Contributor Author

Regarding the code style, it would be nice if you could use more functions, even if it's just creating a main function and then running that.

Ok, I added a "compute_diagnostic" function which is called by the main script.

For now the main script is still that, a script. Still, please notice that it is enclosed in let....end block. This guarantees that the enclosed variable scope is local (and not global as in scripts) and avoids the counterintuitive behaviour which julia>=1.0 introduced in scripts (see JuliaLang/julia#28789 , basically the problem that a variable defined in the script will then not be visible for example inside a for loop). This is a solution which I sort of like and works well. Another option to avoid the issue would be indeed to define a main function and call it. Preferences ?

@bouweandela
Copy link
Member

Sorry for the slow reply. I have preference for a main function, because it sets a good example of using functions in diagnostic code.

@jhardenberg
Copy link
Contributor Author

Hi @bouweandela , sorry I was even slower in reacting to this. The diagnostic is now using a main() function as requested. Please let me know what you think. Cheers.

@bouweandela
Copy link
Member

Hi Jost, looks great to me.

Maybe it would be nice if you could add some example code that reads the netcdf file and creates a plot, just to get people started and make them aware of the preferred libraries to use for this. If you don't have time for this now, we can also merge as is.

Also a mention of the example script in the documentation would be nice, e.g. in doc/sphinx/source/esmvaldiag/new_diagnostic.rst?

@jhardenberg
Copy link
Contributor Author

Sure @bouweandela I will try and add these things asap

@jhardenberg
Copy link
Contributor Author

Hi @bouweandela , the example is now much more sophisticated, as it reads a netcdf file, computes a temporal average, writes this as output file and produces a nice plot.

The only thing which is not working as expected is provenance: my diagnostic_provenance.yml contains a "plot_file" entry pointing to the plot file, but still the plot file does not get any provenance attached. I cannot see why ....

@jhardenberg
Copy link
Contributor Author

PS: since plotting a map is a general issue, I added a function for this in diag_scripts/shared/external.jl or would it be better for it to have its own file like diag_scripts/shared/plotmap.jl ?

@jhardenberg
Copy link
Contributor Author

I added a reference to the julia example in doc/sphinx/source/esmvaldiag/new_diagnostic.rst
Actually I noticed that also R and NCL were missing so tentatively I added those too (this could also be done in a separate PR of course). I wonder if one should add a remark there (when discussing suggested languages) that NCL is not being developed any further ?

@jhardenberg
Copy link
Contributor Author

Hi @bouweandela please see my latest changes above, what do you think (in particular for the documentation changes)? After merging the latest version2_development now this branch also passes al tests. Cheers

@bouweandela
Copy link
Member

Hi @jhardenberg

Thank you for all the updates, I think this now provides a much nicer example to get started.

I added a reference to the julia example in doc/sphinx/source/esmvaldiag/new_diagnostic.rst
Actually I noticed that also R and NCL were missing so tentatively I added those too (this could also be done in a separate PR of course).

Thanks, this is great.

I wonder if one should add a remark there (when discussing suggested languages) that NCL is not being developed any further ?

Yes, that would be good.

The only thing which is not working as expected is provenance: my diagnostic_provenance.yml contains a "plot_file" entry pointing to the plot file, but still the plot file does not get any provenance attached. I cannot see why ....

It seems to work ok-ish: the provenance does get embedded in the .png file. It might be nicer to make a separate provenance record for the plot file (re: our earlier discussion about the plot_file attribute and whether or not this is a good design, should support multiple plots etc.), since I do not expect to have much time to improve this in the foreseeable future and that way the .xml files will also get generated.

PS: since plotting a map is a general issue, I added a function for this in diag_scripts/shared/external.jl or would it be better for it to have its own file like diag_scripts/shared/plotmap.jl ?

Both is fine, when we have more plotting functions at some point it might make sense to put them together in a separate file.

@bouweandela
Copy link
Member

Are there no native Julia libraries for plotting a map?

@mattiarighi mattiarighi merged commit 82788e0 into version2_development Dec 19, 2019
@mattiarighi mattiarighi deleted the julia_example branch December 19, 2019 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

julia example
4 participants