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

Add file argument to get_nml_value #233

Open
jsta opened this issue Jun 13, 2017 · 9 comments
Open

Add file argument to get_nml_value #233

jsta opened this issue Jun 13, 2017 · 9 comments

Comments

@jsta
Copy link
Member

jsta commented Jun 13, 2017

I am thinking that it would be useful to add a file argument to get_nml_value similar to the other get_* functions.

@lawinslow
Copy link
Member

So it would operate directly on a file instead of a loaded nml object?

@jsta
Copy link
Member Author

jsta commented Jun 13, 2017

I think either-or. Consider this block from get_hypsography:

  # if both are passed, glm_nml is used and 'file' is ignored
  if (missing(file) & missing(glm_nml)){stop('glm.nml file path OR glm_nml must be specified')}
  
  if (missing(glm_nml)){
    glm_nml <- read_nml(file)
  }

@lawinslow
Copy link
Member

Would it then also overwrite the old original file as well? Basically,

  # if both are passed, glm_nml is used and 'file' is ignored
  if (missing(file) & missing(glm_nml)){stop('glm.nml file path OR glm_nml must be specified')}
  
  if (missing(glm_nml)){
    glm_nml <- read_nml(file)
  }


  #do a bunch of stuff to glm_nml

  write_nml(glm_nml, file)

@jsta
Copy link
Member Author

jsta commented Jun 14, 2017

I don't follow. Why would get_nml_value be doing things to glm_nml? My understanding was that it simply retrieved values for inspection unchanged. Are you thinking of a function that would combine an nml file and and local nml object and write a new nml file (aka "combine_nml")?

@lawinslow
Copy link
Member

Ah yeah, ignore that. For some reason I was thinking about a function that muted the file in place. Like we were talking about set_nml.

That being said, wouldn't it make sense to implement the parallel version in set_nml? A file could be specified and it would overwrite the NML with a new one with the new variable set. This would eliminate alot of code that reads, alters, then writes NML files.

@jsta
Copy link
Member Author

jsta commented Jul 11, 2017

I am about to submit a PR with this ability added. I was thinking only of reading values not of writing or setting.

I think what you are talking about is more along the lines of #231?

@lawinslow
Copy link
Member

My vote is to parallel this functionality in the set_nml function. But I'll leave this up to @jread-usgs if he wants to merge the PR as-is.

@jordansread
Copy link
Member

Awesome work @jsta
I agree w/ Luke that it would be best to parallel this, but in a different PR if possible. I haven't been able to review (in all-day meetings), but will soon.

@jordansread
Copy link
Member

Yes, I like this idea, and like the parallel idea @lawinslow proposed for set_nml

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

No branches or pull requests

3 participants