First specl function def PR

[rburghol]

Work with @jdkleiner to get a PR on respec with the bare bones specl inclusion:

• function file, with current arguments, but no code.
• code to load function file.
• I recommend omit the calls in _hydr_ (could raise as separate issue, it might be a good conversation starter)

[jdkleiner]

@rburghol Sounds good, I'm working on getting a clean "pull request ready" version of the specl branch now

[jdkleiner]

@rburghol I think I've got it ready for a RESPEC pull request, take a look (See specl branch) https://github.com/HARPgroup/HSPsquared/tree/specl

Only 2 files are modified compared to the master branch of HSPsquared

• Created SPECL.py
  • included a print() call in each of the functions, they just need something in there otherwise HSP2 gets mad

• Modified HYDR.py:

  
  from HSP2.SPECL import specl, _specl_
  
  # commented out specl() call at the beginning of the HYDR for loop
  # call specl
  # specl(ui, ts, step, specactions)
  

Other files with specl stuff (I did not include these file updates in this push):

• main.py
• uci.py
• hdf.py
• ParseTable.csv
• readUCI.py

Note: specl is the "pull-request ready" branch, I've stashed all of our additional specl progress in specl_10.18.22

[rburghol]

Awesome, I'll take a quick look at it compared to the develop branch that we have locally and let you know, but it sounds great.

[rburghol]

By the way I really like how you set all of the additional changes into a separate branch so that we can just make the poll requests from our specl branch. I think that'll help organize it really nicely.

[rburghol]

Thanks again -- Here are my suggestions/questions:

• I think that:
  • ts = _specl_(ui, ts, step, specactions)
  • should be
  • errors = _specl_(ui, ts, step, specactions)
  • Note: ts should be passed by reference so no need to return it as it is edited in place
• The whole errors is probably something that we need to request guidance on, as the errors array seems to be customto the domain, that is, hydr() has errors keyed 0,1 and 2 (there are a couple of spots where they increment the count in errors[0], errors[1], or errors[2], and thus they seem to have some very specific meaning ... maybe they reference the strings in ERRMSGS?
• Maybe we don't need the _specl_() function at all? It seems that the convention is for the _fun_() counterpart to fun() to be the part that is called with @njit, and since we call specl() with @njit -- rightly I think since it is called by the @njit function _hydr_()

[jdkleiner]

@rburghol After a little bit of sleuthing I agree. Because we're calling specl() from within the njit-decorated _hydr_(), we probably don't need the second level _specl_() so long as all of the processes within specl() are "Numba compile-able".

Excerpt from RESPEC HSPsquared documentation _"The reason for the two functions levels is that Numba does not compile all Python language constructs, any Pandas functions, and some Numpy library functions. So the top level function, snow() for example, can be arbitrary Python, Pandas and Numpy code which prepares data to be used by the second level code like _snow_( ). The second level code and all routines called by it must be compatible with Numba so that the code can be compiled. Any violation of Numba constraints will prohibit the compilation and force the code to run a Python speed."_

• https://github.com/respec/HSPsquared/wiki/HSP2_Design_Details#basic-information

I'll work on revising specl accordingly.

Perhaps the function becomes this simple, is @njit needed if its called within a @njit-wrapped function? :

def specl(ui, ts, step, specactions):
    print("specl()")

    # example for modifying ts element
    ts['OUTDGT2'][step] = 99

[rburghol]

Awesome:

• I don't know if @njit is required, but I was aasking myself the same question, and looked at the demand() function, since it is called by the @njit enabled _hdyr_() and it does indeed have @njit, so I think that's reason to include @njit.
• It is interesting to see that they already have at least one routine, snow() that does not use it. I think we should start with @njit, and then all our code should be compatible, then later on if we find that we need to use python functions that are outside of the @njit scope. Better than getting the whole thing done, realizing it runs too slow, then having to go back and refactor to work with @njit.
• Can you add that documentation link somewhere in our development issues? That is good info!

[rburghol]

I have onemore thing:

• the print for every line of execution is too verbose for inclusion in an actual branch. Even if it is not called. Can we put something like a meaningless variable definition in there?

Also I noticed something interesting.

• Compare specl to respec/master and it says can be merged automatically, see: https://github.com/respec/HSPsquared/compare/master...HARPgroup:HSPsquared:specl
• Compare specl to respec/develop and it says it CAN'T be merged. Any ideas? They look identical to me: https://github.com/respec/HSPsquared/compare/develop...HARPgroup:HSPsquared:specl

[jdkleiner]

@rburghol

• I don't know if @njit is required, but I was aasking myself the same question, and looked at the demand() function, since it is called by the @njit enabled _hdyr_() and it does indeed have @njit, so I think that's reason to include @njit.
  • ans. I noticed the same thing with demand(), so I've included @njit(cache=True) before the specl() definition
• It is interesting to see that they already have at least one routine, snow() that does not use it. I think we should start with @njit, and then all our code should be compatible, then later on if we find that we need to use python functions that are outside of the @njit scope. Better than getting the whole thing done, realizing it runs too slow, then having to go back and refactor to work with @njit.
  • ans. I had a typo when I copied over that quote. snow operates the same as hydr i.e. snow() & _snow_(), hydr() & _hydr_()
• Can you add that documentation link somewhere in our development issues? That is good info!
  • ans. will do!

See updated branch here (also removed print): https://github.com/HARPgroup/HSPsquared/blob/specl/HSP2/SPECL.py

[jdkleiner]

Also to your merging question. Looks like we've already pushed an earlier version of SPECL to the respec develop branch?https://github.com/respec/HSPsquared/blob/develop/HSP2/SPECL.py

[rburghol]

Ahhhhh. Maybe that's why the merge is funky. OK - so that's weird. I honestly don't know how that happened. My PRs have only been with the branch psa, but looks like I pulled specl into that? That was not a thoughtful thing for me to do :). At least the code is not filled with testing stuff.

So:

• Can you tell if it includes the "include" lines?
• Should we merge psa with your specl, and then see what's up? Or re-add your include and cleaned up code to the current develop branch?

Sorry this is getting complicated.

[jdkleiner]

Yeah somehow the psa branch has all of the other specl development stuff in there (multiple additional files modified with specl stuff beyond the simple example of HYDR.py just loading and calling specl() and SPECL.py defining specl()).

So perhaps place your latest psa work into the specl branch, then do a pull request to respec master? or is the develop branch the place to do all pull requests?

[rburghol]

Oh sheesh -- thanks for sleuthing that out.

• Do all PRs from specl: I think that IS the way to go. That is what I should have done in the first place.
• develop branch is place to do all pull requests: yes, definitely. This is what the respec folks requested.
• merge psa into specl: yes, this is the way. But before we do that, should we take a copy of our specl branch, then replace our specl with develop, since changes have already been merged and I synched our local develop this morning (when I was testing your specl)?

[jdkleiner]

• merge psa into specl: yes, this is the way. But before we do that, should we take a copy of our specl branch, then replace our specl with develop, since changes have already been merged and I synched our local develop this morning (when I was testing your specl)?
  • I "think" that approach is sound. Would we keep all the additional specl stuff in that new specl branch we're pull requesting? (special actions things in main.py, uci.py etc.)

[rburghol]

• Would we keep all the additional specl stuff in that new specl branch we're pull requesting? (special actions things in main.py, uci.py etc.)
  • Yes. Sort of. Those things should be in the develop branch, so if we clone the develop branch to be our new specl, then these things should already be in there. Then, you could simply copy your HYDR.py and SPECL.py files, and game over. Maybe?

[jdkleiner]

Ok gotcha, I'll do just that and report back

[jdkleiner]

Alright I believe I've got it, see the updated specl branch: https://github.com/HARPgroup/HSPsquared/tree/specl/HSP2

Though still unable to merge with respec/develop?

[jdkleiner]

Pull request made: https://github.com/respec/HSPsquared/pull/104