-
Notifications
You must be signed in to change notification settings - Fork 65
Feature / julia implementation #237
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
base: main
Are you sure you want to change the base?
Feature / julia implementation #237
Conversation
|
Putting a draft PR here. The files I committed still need proper documentation. However, I believe all of the necessary functionality is present to be able to test the files. Another modification (which I hope to include before merging this PR) is turning the operator files into modules, for better organization and importing within the Julia files. |
|
@valeriabarra I'm setting this PR ready for review, however I know you also requested that documentation be made with a tool like Documenter.jl. I'm still in the process of setting up Documenter for this new Julia implementation, but I have at least included docstrings for the operator files and example file. To all reviewers, please let me know if you have any questions. Thank you. |
jbrzensk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mathgeek31415 , thanks for tackling this. There is a lot of good things here. I added some comments in the code about some small specifics.
On a larger scale, I am thinking this needs to be broken into "src" and "examples" like the other languages. Maybe? we are having a meeting today and I will review with them.
Also, based on how long it took me to run the code, we definently need some documentation, mostly for how to get started. Whether that is adding to the README.md, or just putting a link to another README.md you have in the Julia directory, we need some explanation of how to load and run the code, and what needs to be installed.
I ran into a handfull of errors, don't know if that is because of my julia version (1.11.2), but we also need to decide on a base version to support.
But, thank you very much for contributing. A lot of this is on us to make the big decisions, but for now, this looks terrific. I'll update when we know more.
julia/MOLE.jl/src/MOLE.jl
Outdated
| @@ -0,0 +1,10 @@ | |||
| module MOLE | |||
|
|
|||
| include("Divergence.jl") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be lower case, throws an error in Linux:
LoadError: SystemError: opening file "/home/mole/julia/MOLE.jl/src/Divergence.jl": No such file or directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbrzensk Sorry, I actually intended for the filenames to have a capital first letter. However, I started the files with a lowercase name, and didn't properly update the files through git (e.g. using git mv). I have hopefully corrected this in the update I will push later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mathgeek31415, thank you for this tremendous contribution! Sorry for my delayed comment, but I actually prefer that the file names start with a lowercase letter (it's more customary this way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @valeriabarra, got it, thanks for the feedback! I just pushed a new commit with this update accordingly.
| @@ -0,0 +1,35 @@ | |||
| """ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
""" creates the error:
ERROR: LoadError: cannot document the following expression:
using Plots
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, there needs to be a space between the docstring and using statement! This was definitely my fault, I obviously didn't double check the file after adding in the docstring. This should be fixed in the update I will push later.
julia/MOLE.jl/examples/elliptic1D.jl
Outdated
| # Plot result | ||
| p = scatter(grid,U,label="Approximated") | ||
| plot!(p,grid,exp.(grid), label="Analytical") | ||
| display(p) No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where to put this, but if you run this in one shot from the command line:
julia --project=. examples/elliptic1D.jl
the plot is not shown. Either give explicit instructions on how to run from the Julia prompt, or add a stop that forces the render.
Just an example, but could add this to the end of the script:
println("\nPress Enter to close the plot and exit.")
readline()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @jbrzensk , this goes into the overall idea of adding a README (which I should have done, sorry). I've added the lines you suggested.
| @@ -0,0 +1,5 @@ | |||
| name = "MOLE" | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder: we need to actually generate the uuid, date, etc for this. @valeriabarra would know best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jbrzensk. Is this information you would want to have before considering a merge?
@valeriabarra, I know there is a UUID Julia library, should this be used to generate the UUID? Or is there a specific format/sequence that should be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mathgeek31415 , thank you again for your tremendous contribution. I would start by reading the Julia documentation for packaging and environments.
Now, you have already gone one step ahead. But essentially, at the very beginning of your work, you could have used the generate function in the pkg (]) environment, as:
julia> ]
pkg> generate MyNewPackageAnd it would have created and assigned the UUID. I think you can still do it, but be careful that it might overwrite your existing Project.toml file. So place a back-up copy in a secure location before attempting this, please. Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @valeriabarra, I think I've managed to re-generate the package with a UUID now included.
julia/MOLE.jl/test/testDivergence.jl
Outdated
| @@ -0,0 +1,14 @@ | |||
| using Test, LinearAlgebra | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpaolini @jcorbino This is a good replica, but we need a real test. @mathgeek31415 don't worry about this comment :)
|
@cpaolini we also need a workflow to test this. |
|
@jbrzensk I've pushed an update, per our conversations above. Additionally, I've included (the beginnings of) a README file. I would appreciate any feedback on how I can make the README clearer. My apologies again for not originally including any instructions; this is definitely something crucial that should have been included. Hopefully everything else has been addressed, but please let me know if you find any other errors. Thank you! Edit: Also changed the filenames for the operators to be all lowercase, and updated the names in the MOLE.jl file. |
julia/MOLE.jl/README.md
Outdated
| ## Using MOLE.jl at the command line | ||
|
|
||
| To run a Julia script at the command line using MOLE.jl, To use MOLE.jl in a Julia REPL, navigate to the location where MOLE has been cloned to, then navigate to ```mole/julia/MOLE.jl```. Once here use the following command (here, we will run "elliptic1D" from the examples directory): | ||
| > ```julia --project=. examples/elliptic1D.jl``` No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before being able to run an example, we need to tell users to instantiate/precoimpile the package. Here are the instructions from the CLI. You can translate to similar instructions from within the REPL for the section above (that means, first tell them to switch to pkg (]) mode and then type instantiate and precompile).
julia --project=. -e 'using Pkg; Pkg.instantiate()'
julia --project=. -e 'using Pkg; Pkg.precompile()'
julia --project=. examples/elliptic1D.jl
julia/MOLE.jl/test/testDivergence.jl
Outdated
| @@ -0,0 +1,14 @@ | |||
| using Test, LinearAlgebra | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't comment at the folder level (only on a specific line of code in a file), but inside the test/ folder, we want to have a runtests.jl file that collects all the different tests. It is very common to group tests with the @testset macro followed by a descriptive string for that group of tests and the begin and end keywords to enclose the group of tests.
If we are envisioning to have multiple separate unit testing files, we can have a table-of-content style list in the runtests.jl file (see ClimaCore's runtests.jl for instance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback @valeriabarra , I re-structured the test directory to include a runtests file that aggregates the separate test files for the operators. From the MOLE.jl directory, a user should now be able to start the REPL (using, e.g. julia --project=.), enter the "pkg" mode, and run test to start the different unit tests.
I will put this into the documentation, but just wanted to copy the instructions here too.
Hi @mathgeek31415 thank you again for this amazing contribution! Yes, it would be nice to have (at least a first initial draft) of the documentation via I think we will end up having two separate HTML-based documentation web pages, one for the base MATLAB and C++ implementation (which uses Sphinx and is hosted on ReadTheDocs), and one for the Julia implementation. The latter will be linked from the ReadTheDocs one. Similar to what was done for the Unless we can invest some time in learning if the |
Just to circle back on my previous comment, for now, it is better to keep things simple. We want to pursue the simple Julia documentation via |
Thank you for the update @valeriabarra , I will work on getting a first draft of the documentation via |
|
Hi @jbrzensk @valeriabarra ,
In addition to any errors encountered while testing, it would be great to have feedback on the following:
Thank you very much for your help! |
What type of PR is this? (check all applicable)
Description
This is a starting point for a Julia implementation of the MOLE library. Included are the 1D gradient, divergence, and laplacian operators, an implementation of the elliptic1D example from the MATLAB codebase, and unit tests.
Related Issues & Documents
QA Instructions, Screenshots, Recordings
Once the code is pulled, navigate to the mole/julia/MOLE.jl directory. Then, you can access any of the unit tests in the test/ folder, or you can run the example in the examples/ folder. Start a the project in a Julia REPL using
julia --project=.which will bring in all the necessary packages utilized in the files.Added/updated tests?
_We encourage you to test all code included with MOLE, including examples.
have not been included
Read Contributing Guide and Code of Conduct
[optional] Are there any post deployment tasks we need to perform?
Just remember to start the MOLE project in the Julia REPL with `julia --project=.``
[optional] What gif best describes this PR or how it makes you feel?