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

CA with production #8

Merged
merged 15 commits into from
Oct 3, 2023
Merged

CA with production #8

merged 15 commits into from
Oct 3, 2023

Conversation

jhidding
Copy link
Contributor

Burgess2013 model with only CA and production

@jhidding jhidding marked this pull request as draft September 20, 2023 15:50
@NiklasHohmann
Copy link

NiklasHohmann commented Sep 21, 2023

Please provide some documentation on the contents of the output files. The hdf5 file is currently a large 4d array. From context it is clear to me which dimension corresponds to the facies, time step, and spatial coordinates. However, I think it'd be best of this info is directly associated with the array, so the interpretation of the data independent is not dependent on the contextual knowledge of the examiner

@NiklasHohmann
Copy link

NiklasHohmann commented Sep 21, 2023

I run into an issue while precompiling the dependencies. I get the error

ERROR: The following 1 direct dependency failed to precompile:

LanguageServer [2b0e0bc5-e4fd-59b4-8912-456d1b03d8d7]

Error: Missing source file for LanguageServer [2b0e0bc5-e4fd-59b4-8912-456d1b03d8d7

(CarboKitten) pkg> resolve
ERROR: expected package `LanguageServer [2b0e0bc5]` to exist at path `\home\johannes\.julia\dev\LanguageServer`

which seems to be due to a hardcoded path in manifest.toml

@jhidding
Copy link
Contributor Author

I run into an issue while precompiling the dependencies. I get the error

ERROR: The following 1 direct dependency failed to precompile:

LanguageServer [2b0e0bc5-e4fd-59b4-8912-456d1b03d8d7]

Error: Missing source file for LanguageServer [2b0e0bc5-e4fd-59b4-8912-456d1b03d8d7

(CarboKitten) pkg> resolve
ERROR: expected package `LanguageServer [2b0e0bc5]` to exist at path `\home\johannes\.julia\dev\LanguageServer`

which seems to be due to a hardcoded path in manifest.toml

This is so annoying. I'm guessing VS Code is adding this entry

@EmiliaJarochowska
Copy link
Member

I had the same but other than giving an error message it doesn't actually interfere with running, at least the examples 🤷‍♀️

@jhidding jhidding marked this pull request as ready for review September 27, 2023 13:29
@EmiliaJarochowska
Copy link
Member

One weird thing that happens to me is if I run:

julia> include("examples/plot-caps-osc.jl")
ERROR: error in method definition: function CaProd.main must be explicitly imported to be extended

and include("src/CaProd.jl") doesn't help, but if I open and run CaProd.jl in VS Code using Julia: Execute active File in REPL, then CaProd.main is imported and I can run include("examples/plot-caps-osc.jl") 🤷‍♀️

Copy link
Member

Choose a reason for hiding this comment

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

Nice idea, but are the plots the benchmark for comparing the output for different settings of CA? I found the plot very hard to read but maybe I miss something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just not done yet ;)

@NiklasHohmann
Copy link

NiklasHohmann commented Oct 2, 2023

Adjust README

  • use template funding info & author info from template repository in the MtG orga
  • add repo structure with brief descriptions of what does what
  • correct typo in schlager (there is an h missing in the README)

@NiklasHohmann
Copy link

  • Add minimum running instructions for ca-with-production. As is, it is not obvious for an outside user how to run the model, and how to modify it
  • Include tests that outputs from ca-with-production match outputs from the Bosscher-Schlager model

@EmiliaJarochowska
Copy link
Member

I think some of these things we can fix ourselves if @jhidding is comfortable with that?

@jhidding
Copy link
Contributor Author

jhidding commented Oct 2, 2023

Some of these have little to do with this PR. I'll fix those items that do, then we merge into main and move on from there?

@EmiliaJarochowska
Copy link
Member

Good for me! I asked Nick and @xyl96 for review this way because I wanted them to confirm that they can run everything. We need a clear yes or no, guys 😉 if you have issues running anything this is the moment to share that 😊

@NiklasHohmann
Copy link

I'm not sure what is considered part if this PR @jhidding . What would you usually check as part of a PR? @EmiliaJarochowska if it is about being able to run the model: As is I'm not able to run the model from the provided explanations.

@jhidding
Copy link
Contributor Author

jhidding commented Oct 2, 2023

Hmmm, let's say this PR is purely about merging CA and carbonate production code into main. In an early stage like this we should accept that documentation and unit tests are not yet upto standard, and changes shouldn't stay out of main branch for too long. That being said, you should be able to run the examples or I do need to add more documentation before merging.

@NiklasHohmann
Copy link

@jhidding and I did some troubleshooting with the namespaces - no more problems from my side with running it

@xyl96
Copy link

xyl96 commented Oct 2, 2023

All the ca (Burgess) stuff runs here. But After I run the Burgess stuff, the bosser schlager 1992 is not working for some reason (I might did some stupid things or have done sth wrong): ERROR: UndefVarError: plot not defined. Why it's not recognising ''plot'' anymore...

@EmiliaJarochowska
Copy link
Member

What is the difference between MODEL1 in Config.jl and Facies in the Input struct:

facies = [
        CaProd.Facies((4, 10), (6, 10), 500.0, 0.8, 300),
        CaProd.Facies((4, 10), (6, 10), 400.0, 0.1, 300),
        CaProd.Facies((4, 10), (6, 10), 100.0, 0.005, 300)
    ],

MODEL1 is used in burgess-2013.jl and in long-term.jl, but other examples have all parameters of Facies in Input. Is MODEL1 only created to reproduce Burgess (2013) and otherwise Facies should be always provided as part of Input?

@jhidding
Copy link
Contributor Author

jhidding commented Oct 3, 2023

MODEL1 is used in burgess-2013.jl and in long-term.jl, but other examples have all parameters of Facies in Input. Is MODEL1 only created to reproduce Burgess (2013) and otherwise Facies should be always provided as part of Input?

Yeah, I want two things: make it easy to reproduce exact figures in BS92 and B13, but also show users how they may go about changing these things.

@jhidding jhidding merged commit fd131d1 into main Oct 3, 2023
@EmiliaJarochowska
Copy link
Member

EmiliaJarochowska commented Oct 3, 2023 via email

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.

None yet

4 participants