Adding function equivalent to ZPL numeric function OBJC($A) in zospy.functions.nce

[Omnistic]

Hi Jan-Willem, and team,

This refers to my answer in the Zemax Community about a ZOSAPI function equivalent to ZPL numeric function OBJC($A).

I somehow didn't manage to run ZOSPy with 2023 R1.03 though, so I couldn't test the example I wrote in the docstring.

I also don't know about unit testing (I'm not a software engineer) but I'm happy to help if you explain me how to do this kind of stuff.

Take care,

David

[jwmbeenakker]

Thanks for you contribution @Omnistic. We are about to push some new functionality to ZOSPy, so this one can swiftly be included the new versions.

Regarding ZOSPy not working with your OpticStudio, we are happy to help/fix the problem. But maybe then it is better to report an issue, so we can follow up (as this pull request will probably soon closed).

[jwmbeenakker]

@crnh Would be willing to implement the unit test? I think the example David gave is already a sufficient test, which can be applied to the simple_system.

[crnh]

@crnh Would be willing to implement the unit test? I think the example David gave is already a sufficient test, which can be applied to the simple_system.

Sure, unless @Omnistic is very eager to learn about unit testing... I think you did already give some hints!

[LucVV]

Hi @Omnistic, nice work! Two questions regarding the implementation:

1. The function currently returns -1 if no objects are found. This probably reflects the behaviour of the macro, but I think that's not required. What about returning None or an empty list? (The latter is common practice in libraries like NumPy and Pandas; check e.g. numpy.where or pandas.DataFrame.query).
2. Instead of a list of ints, you can also return a list of objects, since you're probably going to use the indices to get the objects anyways.

Regarding point 1, I think returning an empty list is preferable. In most cases, you will have to check the length of the returned list anyway befire you do that. Returning '[]' in stead of '-1' will save you from checking the tyoe of the return first. Regarding point 2, I am also in favour of returning the object as you can get the position fron the object. We could also add a parameter to specify whether objects or object positions are returned if we want to support both possibilities.

@LucVV suggested that this function can also be implemented for the LDE. I still need to investigate if we can do this using a shared implementation, or if we need completely separate implementations for each editor.

I have had a similar implementation in a project before to find the position of a surface in the LDE. I think you need to call slightly different API functions so separate implementations might be the easiest. I'll look into this.

[crnh]

I agree with this. Although explicit type checking is not needed in this case (comparing a list with an int will return False, so that would be sufficient), it is still better to limit the amount of possible return types. An empty list therefore also has my preference.

Object indices can be easily found using a simple list comprehension, so I would leave this to the user. I don't think there are many cases in which you really need the indices instead of the objects.

As far as I know, the LDE rows and NCE objects implement the same interface. If the Comment property is part of this interface, we can probably use a shared implementation for both editors. Otherwise, it's indeed better to have two completely separate implementations.

[crnh]

As far as I know, the LDE rows and NCE objects implement the same interface. If the Comment property is part of this interface, we can probably use a shared implementation for both editors. Otherwise, it's indeed better to have two completely separate implementations.

I just checked this. INCERow and ILDERow do indeed both implement IEditorRow, but Comment is not a member of IEditorRow. So we need two separate implementations.

[Omnistic]

Thank you for the comments. The function now returns a list of object that is empty if no match is found. As for the sequential mode implementation of this function, the only difference is that one has to call GetSurfaceAt from the LDE instead of GetObjectAt (from the NCE). Let me know if I should implement this in the same function or if you think we need another one (happy to write it as well).

[Omnistic]

@crnh Would be willing to implement the unit test? I think the example David gave is already a sufficient test, which can be applied to the simple_system.

Sure, unless @Omnistic is very eager to learn about unit testing... I think you did already give some hints!

Not eager definitely, but if you have some pointers for me to have a rough idea of what it means, I'm all ears.

[Omnistic]

Now that I'm able to better run ZOSPy, I noticed both function in nce.py were refering to oss.NCE.InsertNewObjectAt(0) in their docstring. However, OpticStudio counts objects from 1. I think it should be replaced by oss.NCE.InsertNewObjectAt(1).

Also, I'm probably doing something wrong but the example for object_change_type doesn't work for me. I tried adding the wakeup call and changing the new object index to 1, which helped:

import zospy as zp
zos = zp.ZOS()
zos.wakeup()
zos.connect_as_extension()
oss = zos.get_primary_system()
oss.make_nonsequential()
newobj = oss.NCE.InsertNewObjectAt(1)
object_change_type(newobj, zp.constants.Editors.NCE.ObjectType.StandardLens)

But I get a

NameError: name 'object_change_type' is not defined

now. Should object_change_type be called from zp somehow?

[LucVV]

However, OpticStudio counts objects from 1. I think it should be replaced by oss.NCE.InsertNewObjectAt(1).

I see, sorry the example is incorrect. I will adjust it soon! It seems to stem from the equivalent function for the LDE. Somehow,surface numbering starts at 0 in sequential mode but and at 1 in non-sequential mode. I have never fully figured out the reasoning behind that. I do think this is why two separate implementations of your function are required.

now. Should object_change_type be called from zp somehow?

That is correct, object_change_type() is available by calling zp.functions.nce.object_change_type(). Or equivalently by importing it:

from zospy.functions.nce import object_change_type()
object_change_type(...)

Sorry that that is not clear in the example, I will update that.

[LucVV]

Hi @Omnistic,

I have just added the same function for the LDE as find_surface_by_comment(). I have furthermore added a unit test. I think this unit test can swiftly be altered to work for the NCE. However, the simple_system that I use is sequential. We have a nsc_simple_system ready in the next release that might be optimal for this test. @crnh what do you think is the best way to go regarding to that non-sequential simple system?

[crnh]

We have a nsc_simple_system ready in the next release that might be optimal for this test. @crnh what do you think is the best way to go regarding to that non-sequential simple system?

Introducing the nsc_simple_system in this PR as well seems to me the easiest way to add proper unit tests for zospy.functions.nce.find_objects_by_comment. Otherwise we won't be able to test the unit tests...

[LucVV]

I have added the non-sequential simple system for testing purposes.

[jwmbeenakker]

@crnh Could you "quickly" add the unit test for the non-sequential part, so we can include it in the merge with all our internal stuff for version 1.1.0