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

so-tg/preproc #60

Merged
merged 73 commits into from
Nov 12, 2020
Merged

so-tg/preproc #60

merged 73 commits into from
Nov 12, 2020

Conversation

samoliverowens
Copy link
Contributor

This branch contains the new pre-processing setup, which I'm fairly confident produces identical input files as the old setup. There are a few issues (see #56 and #59) that I aim to fix quickly in a subsequent pull request. I will continue to check the new setup works like the old whenever I set up new experiments. I have also written some brief documentation for pre-processing, and I expect to add to this as I use the code more.

This closes issues #30 and #31.

@samoliverowens samoliverowens added documentation Improvements or additions to documentation pre-processing labels Jan 31, 2020
tools/pre/da_inp.m~ Outdated Show resolved Hide resolved
@dmey
Copy link
Contributor

dmey commented Jan 31, 2020

@bss116 let me know if you can review this -- a bit short of time -- may only be able to pick up on major things.

@bss116
Copy link
Contributor

bss116 commented Jan 31, 2020

I would also suggest to move the files one level up directly into tools - there are only the three files da_inp, da_inp.m and da_pp.m left, correct? Also, could you think of more descriptive names for them (I know other people are to blame for this... :D ) and add the .sh ending to the bash script? Then we can also delete all the remaining python scripts, which are now on a separate branch.

@bss116
Copy link
Contributor

bss116 commented Jan 31, 2020

@bss116 let me know if you can review this -- a bit short of time -- may only be able to pick up on major things.

Best one to review this would actually be @ivosuter, I have no clue about the energy balance 😄 . But can give it a quick test and see if I can get it to run on HPC (don't have matlab on my computer) and check the results. I think we have discussed this a lot, I'm confident it will be alright.

@samoliverowens
Copy link
Contributor Author

For the names, would something like da_inp.m -> da_write_inps.m and da_pp.m -> da_preproc_class.m (or just da_preproc.m) make sense?

@bss116 bss116 linked an issue Nov 10, 2020 that may be closed by this pull request
@bss116
Copy link
Contributor

bss116 commented Nov 10, 2020

So I looked through the comments above, there's a few small things that need to be done and then it's ready to get merged.

  • test compatibility of source code (.F90 files) changes with examples, particularly example 001

  • save the default data for walltypes.inp within preprocessing.m, then remove the template file from the tools directory and change write_input_files.m accordingly --> @samoliverowens I'm don't know how simple this is, do you want to look into this now or should I create a new issue for it?

  • change write_input_files.m to be a function, so we can remove the dependency on hardcoded experiment number and environmental variables, so that git does not detect changes every time it is used --> this is an improvement, so I'll create a separate issue for it

  • renaming the bash scripts and updating the docs with the new names --> I'm happy to go over that. Name suggestions below.

  • removing tools/pre directory and namoptions.xxx file --> @samoliverowens, nothing in tools/pre is used any more, right? I created the namoptions.xxx file when writing the namoptions overview. I dragged it along in case it might be useful. I'd just delete it, or do you think this may be useful in the preprocessing?

Name changes:
concatenate_field.sh -> nco_concatenate_field.sh
da_concatenate.sh -> gather_outputs.sh
da_append.sh -> append_outputs.sh
da_prep.sh -> copy_inputs.sh
da_inp.sh -> write_inputs.sh
write_input_files.m -> write_inputs.m? We also discussed update_inputs.m or matlab_write_inputs.sh (or any other prefix...)

@samoliverowens
Copy link
Contributor Author

samoliverowens commented Nov 10, 2020

So I looked through the comments above, there's a few small things that need to be done and then it's ready to get merged.

  • test compatibility of source code (.F90 files) changes with examples, particularly example 001
  • save the default data for walltypes.inp within preprocessing.m, then remove the template file from the tools directory and change write_input_files.m accordingly --> @samoliverowens I'm don't know how simple this is, do you want to look into this now or should I create a new issue for it?
  • change write_input_files.m to be a function, so we can remove the dependency on hardcoded experiment number and environmental variables, so that git does not detect changes every time it is used --> this is an improvement, so I'll create a separate issue for it
  • renaming the bash scripts and updating the docs with the new names --> I'm happy to go over that. Name suggestions below.
  • removing tools/pre directory and namoptions.xxx file --> @samoliverowens, nothing in tools/pre is used any more, right? I created the namoptions.xxx file when writing the namoptions overview. I dragged it along in case it might be useful. I'd just delete it, or do you think this may be useful in the preprocessing?

Name changes:
concatenate_field.sh -> nco_concatenate_field.sh
da_concatenate.sh -> gather_outputs.sh
da_append.sh -> append_outputs.sh
da_prep.sh -> copy_inputs.sh
da_inp.sh -> write_inputs.sh
write_input_files.m -> write_inputs.m? We also discussed update_inputs.m or matlab_write_inputs.sh (or any other prefix...)

Great, I'll have a look at this tomorrow.

@samoliverowens
Copy link
Contributor Author

samoliverowens commented Nov 11, 2020

  • save the default data for walltypes.inp within preprocessing.m, then remove the template file from the tools directory and change write_input_files.m accordingly --> @samoliverowens I'm don't know how simple this is, do you want to look into this now or should I create a new issue for it?

I've just finished doing this, will push to PR shortly.

  • change write_input_files.m to be a function, so we can remove the dependency on hardcoded experiment number and environmental variables, so that git does not detect changes every time it is used --> this is an improvement, so I'll create a separate issue for it

Sounds good to me.

  • renaming the bash scripts and updating the docs with the new names --> I'm happy to go over that. Name suggestions below.

Happy with those names!

  • removing tools/pre directory and namoptions.xxx file --> @samoliverowens, nothing in tools/pre is used any more, right? I created the namoptions.xxx file when writing the namoptions overview. I dragged it along in case it might be useful. I'd just delete it, or do you think this may be useful in the preprocessing?

That's right, it should all be in tools. I don't think namoptions.xxx would be used in the pre-processing because the defaults are set in the class, so it can be removed.

@samoliverowens
Copy link
Contributor Author

* test compatibility of source code (.F90 files) changes with examples, particularly example 001

I can successfully run all the example simulations, including 001. I can see now why 502 doesn't crash despite issue #88 - see my comments there.

I'll now generate some plots!

@bss116
Copy link
Contributor

bss116 commented Nov 12, 2020

Great! Have you also checked with the updated facets (#89)? I suggest to merge #89 first.

@samoliverowens
Copy link
Contributor Author

Great! Have you also checked with the updated facets (#89)? I suggest to merge #89 first.

Yep I was running with the new facet ids, so yes let's merge #89.

@bss116 bss116 self-requested a review November 12, 2020 17:24
@samoliverowens samoliverowens merged commit 3c1b9e4 into master Nov 12, 2020
@samoliverowens samoliverowens deleted the so-tg/preproc branch November 12, 2020 17:35
@dmey dmey restored the so-tg/preproc branch November 12, 2020 17:38
@dmey dmey mentioned this pull request Nov 12, 2020
dmey added a commit that referenced this pull request Nov 12, 2020
@dmey dmey deleted the so-tg/preproc branch November 12, 2020 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation pre-processing
Projects
None yet
4 participants