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

Proposed Node refactoring #728

Open
m3d opened this issue Nov 2, 2020 · 2 comments
Open

Proposed Node refactoring #728

m3d opened this issue Nov 2, 2020 · 2 comments
Assignees

Comments

@m3d
Copy link
Member

m3d commented Nov 2, 2020

It is not obvious that osgar.node:Node automatically fills variables received by self.listen() (for example self.scan when it receives scan). I would propose to add new function register_variables('scan', 'pose3d'), which would automatically define self.scan and self.pose3d, and initialize them to None.

Open question is what to do with self.time, i.e. does it have to be also specified in call register_variables()?

What to do with variables, which are not listed? I would recommend to ignore and not set them.


The second proposed change is to modify Node.update() and extend it to

        handler = getattr(self, "on_" + channel, None)
        if handler is not None:
            handler(getattr(self, channel))

I am using this construct in almost every class.

Note, that this refactoring influences ALL usages of Node including external projects, so I would like agreement before I start it.

@zbynekwinkler
Copy link
Member

I am not sure about the first part - maybe setting any variable is a bad idea. As for refactoring - do you imagine a "big bang" kind of PR that does the changes and updates all users at the same time?

I am thinking about descriptor based solution. Something like

class  ArtifactDetectorDNN(Node):
    image = osgar.input()
    artf = osgar.output()
    dropped = osgar.output()
    debug_artf = osgar.output()
    debug_depth = osgar.output(compress=True)
    stdout = osgar.output()
    debug_result = osgar.output()
    debug_cv_result = osgar.output()

   @osgar.input("image")
   def some_handler(self, ...)...

where the variant image = osgar.input() would just declare the name and handle its setting, while the @osgar.input("image") variant marks the method as a handler. Calling self.dropped.publish(1)' would do what self.bus.publish('dropped', 1)does now. For anyone having experience with pyqt it should be fairly familiar way of working. Therunand/orupdatewould have to be changed accordingly, most likely evolving into something likespinandspinOnce. Also the bus.registercall would be handled insideNode.init. But it is not all roses - I am not sure how to support dynamic outputs in this setup yet (like the once coming from config in Pull` case).

I am not ready to make full fledged proposal yet - I just wanted to let everyone know what I am thinking. In the mean time, I don't mind helping with some improvements to Node.

@m3d
Copy link
Member Author

m3d commented Mar 1, 2023

I was going to create "new" issue and I see it is here already for almost 3 years :) ... maybe it is time. We would like to do more changes (like removal of "application" so that all config files have to be complete and not "parametrized" by application). Finally cleanup to keep only the "core" utilities in osgar and drop/move unused bits ... but let's start with these 2 listed in the original message from 2020.

@m3d m3d assigned jisa, m3d and tajgr Mar 1, 2023
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

4 participants