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

pst.control_data.__setattr__ Is not setting "passed" column in self._df Therefore may not writing optional args #379

Open
hansonmcoombs opened this issue Oct 19, 2022 · 6 comments
Labels

Comments

@hansonmcoombs
Copy link

hansonmcoombs commented Oct 19, 2022

When pst.control_data is accessed via setattr method the passed column in pst.control_data._df is not set, which means that user passed optional control data will never be written to the pest file.

pst.control_data.numcom = 1

print(pst.control_data.passed_options)

print(pst.control_data._df.loc['numcom'])

yields:

  • {}
  • name [numcom]
  • type <class 'numpy.int32'>
  • value 1
  • required False
  • format <function at 0x7f81ac341a20>
  • passed False
  • Name: numcom, dtype: object

Therefore (~pst.control_data._df.passed).all() ==True, always.

This causes a problem in self.write_keyword() lines 481-493

        for n, v in zip(self._df.name, self.formatted_values):
            if n.replace("[","").replace("]","") not in kw:
                if n not in self._df.index:
                    continue
                if not self._df.loc[n, "passed"]: # always TRUE
                    continue
                if self._df.loc[n,"value"] == default_values.get(n,self._df.loc[n,"value"]):
                    continue
            if n.replace("[","").replace("]","") in dimen_vars:
                continue
            if n.replace("[", "").replace("]", "") in unused:
                continue
            f.write("{0:30} {1}\n".format(n.replace("[","").replace("]",""), v))

and also causes a problem in self.write() lines 506-516

    for line in CONTROL_VARIABLE_LINES:
        [
            f.write(self.formatted_values[name.replace("[", "").replace("]", "")])
            for name in line.split()
            if self._df.loc[name.replace("[", "").replace("]", ""), "passed"]  # always False
            == True
            or self._df.loc[name.replace("[", "").replace("]", ""), "required"]
            == True
            or name.replace("[", "").replace("]", "") in self.keyword_accessed  # though it will write because keywords are added
        ]
        f.write("\n")

to fix this in pyemu/pst/pst_controldata.py

in def setattr add the following lines at or before line 261:

self._df.loc[key, "value"] = self._df.loc[key, "type"] (value) # existing line of code 260

self._df.loc[key, "passed"] = True # new lines
self.passed_options[key] = self._df.loc[key, "type"] (value) # not essential, but makes the passed_options attribute useful

note all line numbers are from https://github.com/pypest/pyemu/blob/develop/pyemu/pst/pst_controldata.py on 19-10-2022

I am happy to do a pull request here, but it's a simple enough fix that it might make more sense to have an admin make the change.

Let me know if you have any questions

@jtwhite79
Copy link
Collaborator

I think the passed flag means the variable was set in the ctrl file that was read. But I need to check that. I believe the keyword_accessed container might be what is being used to track options that may have been modified in py...

@hansonmcoombs
Copy link
Author

Ah that distinction makes sense. Then I would suggest changing self.write_keyword() lines 481-493 (change in bold)

From:

        for n, v in zip(self._df.name, self.formatted_values):
            if n.replace("[","").replace("]","") not in kw:
                if n not in self._df.index:
                    continue
                if not self._df.loc[n, "passed"]:
                    continue
                if self._df.loc[n,"value"] == default_values.get(n,self._df.loc[n,"value"]):
                    continue
            if n.replace("[","").replace("]","") in dimen_vars:
                continue
            if n.replace("[", "").replace("]", "") in unused:
                continue
            f.write("{0:30} {1}\n".format(n.replace("[","").replace("]",""), v))

To:

        for n, v in zip(self._df.name, self.formatted_values):
            if n.replace("[","").replace("]","") not in kw:
                if n not in self._df.index:
                    continue
                if (not self._df.loc[n, "passed"]) and (n not in self.keyword_accessed):
                    continue
                if self._df.loc[n,"value"] == default_values.get(n,self._df.loc[n,"value"]):
                    continue
            if n.replace("[","").replace("]","") in dimen_vars:
                continue
            if n.replace("[", "").replace("]", "") in unused:
                continue
            f.write("{0:30} {1}\n".format(n.replace("[","").replace("]",""), v))

@jtwhite79
Copy link
Collaborator

That seems to make sense. Admittedly I havent tested much with all the options in * control data because PEST++ doesnt used many of them (mostly just noptmax) and PEST doesnt support the version 2 format....

@hansonmcoombs
Copy link
Author

makes sense.

@briochh
Copy link
Collaborator

briochh commented Jan 19, 2023

@hansonmcoombs, fancy putting together a PR (and maybe a tiny test) for this?

@hansonmcoombs
Copy link
Author

hansonmcoombs commented Jan 22, 2023

@briochh yep I can aim to get this out relatively soonish I'd probably bundle it with #378

I would also like to add a bit of formal documentation around the control_data class (better linking/describing the control data variables for those of us who haven't memorized Johns pest manuals). Where would the best place to add this information? would you all prefer a longer doc string (it might get quite long) or a new readme.md file in the pyemu/pst folder that is linked to in the doc string, or something else?

Edit: I also want to better document how the controldata class works (e.g. the meaning of the passed variable). I reserve the right to call you and pick your brain.

@briochh briochh added the pst label Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants