Fix the use of Pset arguments when calling a task in PyRAF CL

[olebole]

This implements the suggestion by @jehturner in #192 to explicitly propagate Pset arguments to the main dictionary.

Closes: #192

[jehturner]

My suggestion was certainly a bit of a hack, as __pardict is accessed through getter- & setter-type interfaces elsewhere, but I hadn't quite figured out what the usage should be. I did wonder whether the p.get().getParObject(tail) in setParList() should return something linked to the axis parameter from the top-level list (as per your #192 (comment)) and whether that's what's broken (which sounds related to the comment in __addPsetParams()).

[jehturner]

I wonder about something like this?

            if tail:
                # is pset parameter - get parameter object from its task
                p = p.get().getParObject(tail)
                try:
                    p = self.getParObject(p.name)
                except KeyError:
                    p = copy.deepcopy(p)
                    self.addParam(p)

Some thoughts:

• While I've only studied the mechanism to a limited extent, the main parameter list seems to be what's used to construct the message sent to the IRAF process.
• It sounds like the pset parameters were originally linked to the main parameters, but were severed at some point when a design problem became apparent (perhaps related to CL script namespaces).
• Any resulting duplication between the main list and the pset appears to be harmless, having already existed for a long time.
• Since this version doesn't re-establish the link, it avoids any resulting problems (whatever they might be).
• I'm not 100% sure whether the equivalent parameter is guaranteed to exist in the main list by the time this code runs, hence the fallback to addParam(). This is probably unnecessary, but at worst is a couple of redundant lines, to be safe, which we can remove if you are confident that the entry should be there (this isn't very well-tested, since I don't have a real-world case).
• This uses the API, rather than editing private attributes in more places than necessary...

[jehturner]

Ideally, we'd have a test for this, of course (I'd need to look at that later if you want me to add one). Sorry, I messed up slightly above (the deepcopy was a last-minute addition); let me just fix that [done].

[olebole]

I think the parameter is guaranteed to exist because it was added during init by self.addParm() which adds the Pset to self.__psets2merge, from which it is later (by __addPsetParams()) added to the main list; however it cannot hurt to increase robustness by adding the try…except clause as you propose.

So, I added a new commit (and set you as the author to both to make clear who gets the merits :-) ). Ofcourse a test would be great so that we can catch a regression if the needs to be changed again. Do you mind to be added as maintainer so that you could modify the PR yourself? Hidden thought is that it is better to have more people with access to the repository to reduce the bus factor 🚌

[jehturner]

Sure, thank you!

[olebole]

@jehturner I tried to formulate a few tests for the PyRAF parameter handling. However, surprisingly for the these tests pass on the main branch but fail in this PR.

The test uses a test task, showparams, which accepts one param for int, float, str, bool and ParameterSet, and just prints all the parameters:

--> task showpars = "pyraf/tests/testpkg/showpars.cl"
--> task psetpar = "pyraf/tests/testpkg/psetpar.par"
--> showpars
strpar=unset
intpar=1
fltpar=1.0
boolpar=no
psstrpar=
psintpar=0
--> showpars intpar=2 strpar=foo
strpar=foo
intpar=2
fltpar=1.0
boolpar=no
psstrpar=
psintpar=0

I would expect that setting psetpar.psintpar=2 would not work in the main branch, but work in this branch. However, in this branch the parameter sets are just ignored:

--> showpars psetpar.psintpar=1
strpar=unset
intpar=1
fltpar=1.0
boolpar=no
psstrpar=
psintpar=0
--> showpars psintpar=1
strpar=unset
intpar=1
fltpar=1.0
boolpar=no
psstrpar=
psintpar=0

In CL, it works as expected.
I also made the same exercise with a similar SPP program, with the same result (I just need to include compilation in the test file before I push the update).
Do you have any idea what is wrong with my tests?

[olebole]

Digging a bit deeper with the SPP variant: The SPP code

    psetpar = clopset("psetpar")
    call clgpsets (psetpar, "psstrpar", psstrpar, SZ_LINE)
    psintpar = clgpseti (psetpar, "psintpar")

doesn't work with this PR. This is however the documented way.

gfit1d goes a different way:

    call clgstr ("psstrpar", psstrpar, SZ_LINE)
    psintpar = clgeti ("psintpar")

which works with this PR. Both work with IRAF CL. I don't know whether the sequence in gfit1d.x is formally correct.

What also does not work in both cases is

--> sppshowpars psetpar=psetpar1

i.e. replacing the complete parameter set with another one. This also works for the CL variant.
So, this is still somehow buggy...

[jehturner]

Ugh. I don't have much insight into this off the top of my head, but will have another look. Presumably this usage was also working in PyRAF 2.2.1 & previously, if it works in the main branch, so the regression might cause real-World problems. While I was using gfit1d as a point of reference (because it was the thing failing), I suppose that was written by STScI rather than NOAO, so might not represent the typical usage.

[jehturner]

This issue arises because the code manipulates pset parameters both at the top-level and under the pset. I'm a bit fuzzy on the intention & the convoluted mechanics of all this, but it seems clear why my proposed change solves the gfit1d case while breaking your test...

Before this PR, the code was setting the the IrafPar object nested under the IrafParPset (eg. samplepars.axis) to the determined value, which did not work in gfit1d, because that task looks at the top-level version of the parameter (axis) instead, as you have noted above:

pyraf/pyraf/irafpar.py

Lines 1036 to 1041 in 9607ad7

	if tail:
	# is pset parameter - get parameter object from its task
	p = p.get().getParObject(tail)
	# what if *this* p is a IrafParPset ? skip for now,
	# since we think no one is doubly nesting PSETs
	p.set(value)

My change instead sets the IrafPar for the like-named, top-level parameter (axis), which fixes gfit1d, but I hadn't appreciated that gfit1d is not following the documented convention for accessing parameters that you posted above. It seems that when your showpars looks at psetpar.psstrpar etc., it must be getting the copy under the IrafParPset object (which is therefore needed after all).

Comments in the code suggest that each pset parameter was originally linked by reference to the top-level one, but that link was severed at some point due to architectural problems, leaving these duplicate copies (only one of which has been getting set properly).

Adding a second p.set(value) before the new p = self.getParObject(p.name), to set the other copy as well, made your tests pass for me, but I'm wondering now about any unintended consequences of setting the top-level copy... To complicate things even further, while I was able to reproduce your failure earlier, the test bizarrely seems to be working as-is just now, which probably means it's time to go home for the day!

[olebole]

I added the p.set(value) to the change. I also added p.setFlags(_cmdlineFlag) to keep them the flags in sync.

I also completed the tests for parameter setting, and almost everything works now. Whow!

There seems to be one more issue: In IRAF CL it is possible to set all values on one parameter set with the values of another one. With the testpkg:

ecl> testpkg

# psetpars0 uses the "simplified" access to the parameters, like **gefit1d**
testpkg> psetpars0
psstrpar=bar
psintpar=3
testpkg> psetpars0 psetpar=psetpar1
psstrpar=foobar
psintpar=-1

# psetpars1 uses the "official" method, as described in pset.pdf.
testpkg> psetpars1
psstrpar=bar
psintpar=3
testpkg> psetpars1 psetpar=psetpar1
psstrpar=foobar
psintpar=-1

This does not work under PyRAF, and that's why the tests currently fail. However, I have no idea how relevant this is -- if Gemini doesn't use this, I would suggest to just mark these failures with xfail for the moment. I could imagine that this never worked in PyRAF (I could dig out old versions to check through).

[jehturner]

That case seems to be working for me! 😵‍💫 Thank you for adding some proper tests 🙏.

I was studying this a bit more and the issue was basically that the try block at L925 calls getParObjects(key), which gets both the top-level and pset parameter objects when the argument is specified in the unqualified form (psstrpar), but when it's prefixed with the pset namespace on the command line (psetpar.psstrpar), the code goes into the except, which gets only the pset parameter object with getParObject(), ignoring any top-level version. The try part used to look the same, but was fixed by STScI after a help call (I think from us), which seems to have resolved 3 of the 4 permutations of qualified / abbreviated syntax in the IRAF task / command line, with the remaining case being #192.

The most consistent fix would therefore be to have the except also look for a top-level parameter and stuff it into fullkw if necessary (which for the try happens in the loop over dupl_pset_pars below), but I think what you currently have is equivalent to that (there's a potential issue with matching abbreviated names, but I don't think that's a problem here). Ugh! At the end of the day, the proof of the pudding is in the testing.

PS. getParObjects is documented to fail if they key doesn't exist at the top-level (it always seems to in practice, but I'm not sure that's guaranteed), so it can't just replace getParObject() without a fallback in the except.

[olebole]

I am not sure I can follow...
Does it mean that I should just mark them as xfail and merge the PR?

[olebole]

I just checked out version 2.1.15 (with Python 3) which was the last STScI one. It turns out that it has exactly the same failures as the current main branch:

• settig psetpar.psintpar=1 doesn't work for the "simple" access (that is the one lite gfit1d),
• setting the whole parameter set psetpar=psetpar1 doesn't work for both SPP tasks (official and simplified access).

So we are digging in a really old problem here, if this is a regression at all.

[jehturner]

Yes, these are really old problems, but the first one appears to cause a long-standing problem with error propagation for certain instrument modes in Gemini IRAF, which Kathleen only recently identified. I'm not aware of any specific failure on our side related to the second bullet, but indeed it should be working. I think I have some idea what's going on with that (it looked to me like the pset parameter's value is getting changed without reloading the actual IrafParPset that gets returned by getParObjects()), but I have been asked to work on something else ASAP, so I'd have to finish following that up later if you're worried about it. Thanks a lot for testing this much more thoroughly.

[olebole]

I merged this PR and plan to create a new release. I think @KathleenLabrie mentioned that there are some issues with the 64 bit compatibility of PyRAF compared to IRAF CL. Shall these be resolved before a new release? Then it would be good to have them raised asap.

[jehturner]

Thanks, I'll re-run all our tests on this then. One slight concern is that there are code comments suggesting that there could be problems with naming collisions between pset parameter names and local variables in CL scripts (seems unwise), but we'll see what actually happens.

Kathleen found a GKI-related issue (in addition to this one) that it looked like you might already have fixed, but I have not yet been able to reproduce it with any version of PyRAF. I had asked her for more input, but she has been quite wrapped up with the next DRAGONS release, given our constraints. I can mention that it may miss a release if not identified soon.