Create model features and edit properties

[john-hen]

As suggested by @max3-2 in #16, and further discussed there, it would be convenient to have an easy way to add certain model features, such as global functions or selections etc., and edit their properties. Currently, this is only possible by using the (pythonized) Java methods directly.

Properties are the options displayed on the "Settings" tab in the Comsol GUI. They are accessible through the Java API via various getter and setter methods, depending on the data type they require or return.

The (tentative) proposal is to add three new methods to the Model class:

• create(category, feature) would add a new feature with the given name to a category of feature collections, such as "function" or "selection".
• property(category, feature, name, value=None) would change or return a named property of a feature in one of the categories. The value (passed in or returned) might be anything from a string to an array.
• remove(category, feature) would remove a named feature.

This only covers actual properties, defined as such by Comsol's API. Feature methods that trigger an action are not included.

[max3-2]

Thank you for the roadmap on this one. While I think the property() and remove() are fairly straight forward, I see some difficulties with implementing the category aspect in create() consistently. The java API works like, e.g. a function or a result plane

self.java.func().create(tag, 'Interpolation')

self.java.result().export().create(tag, 'Data')

thus, the user needs to know the right category and the "name" specifying the type. Additionally, there would be the need for a selecting procedure selecting the right category (e.g. func() or result()) from a user input (probably a string). Currently, I only see this being possible by hard coding each feature (or at least each category, and then handling exceptions if the feature "name" is wrong)

[john-hen]

You're right, the create() method would also need to know the type. So the signature should be create(category, type, feature). Or feature could just be name here, as it is the user-supplied name to be assigned to that feature. Then type would be something like "Interpolation" for features from the category "function", or "Adjacent" (or whatever the Comsol API calls it) for the category "selection".

There sure would be a lot of ifs and elifs based on what string the user supplied for category. And raise ValueError if the category string is not recognized. Though if the type string is not valid, Comsol would raise the exception. So we don't have to deal with that. Same if the property name is invalid.

Again, this is certainly not the cleanest approach, as we're using strings to map a to a complex object hierarchy. On the other hand, Comsol also uses strings for parts of that hierarchy, namely the type when creating a feature. We'd only have to document which strings are valid for category. For anything else the user would be referred to the Comsol documentation.

I could be overlooking something. Maybe this is all more complicated than I now naively think it is. It would help to have a use-case example. Like, what's the simplest bit of useful Python code that would create a certain feature and modify some properties, but doesn't have to deal with any Comsol specifics other than the "categories" that we'd document in MPh.

[max3-2]

I can fix something up for an interpolation and an export to have some ideas. A lot can be done with property() I guess. Just an idea without too much code yet: What if we wrap possible categories into sort of a dict - that would make the selection based on keys (and KeyError) - however the value would probably be something like lambda model: model.java.category() (not pretty) and then dict[catetgory_string](self).create() (even less pretty). It would be easily expandable.

Or this could be the point where a class customization could be useful, to add that selection functionality into the java class itself...? So that a call to something like model.java.category_by_name() might be possible

[max3-2]

This would be a quick idea for a decoration. While this is probably not much clearer / less code than in the model class, it moves it out of the way.

        # This could be moved out of the way
        # Decorate the class to act like a dictionary
        @_jcustomizer.JImplementationFor('com.comsol.model.Model')
        class _J_comsol_model:
            def __jclass_init__(self):
                self._categories = {
                    'results': self.result()
                }

            def __getitem__(self, item):
                try:
                    return self._categories[item]
                except KeyError:
                    logger.error('This category has not been implemented yet.')

Warning: This is not working yet. Im getting the slightly cryptic error:

TypeError: No matching overloads found for *static* com.comsol.clientapi.impl.ModelClient.result(), options are:
    public com.comsol.model.Results com.comsol.clientapi.impl.ModelClient.result()
    public com.comsol.model.ResultFeature com.comsol.clientapi.impl.ModelClient.result(java.lang.String)

which I do not really understand since my call is under the options. I guess the issue is with the *static* part. If this is a viable option, maybe @Thrameos could find the time to give a little insight if and how this is fixable.

[john-hen]

I don't know either. Usually this error means that you're calling a class method when only an instance method is available. But that isn't the case here. Maybe it has something to do with the fact that com.comsol.model.Model classes cannot be instantiated directly. That raises TypeError: Cannot create Java interface instances. I'm guessing that they are actually created by ModelClient, acting as a factory.

Maybe it's better to use the Python dictionary. To keep things simple. Or an access method, much like Model._dataset() now. The dictionary primarily saves typing and avoids repetition of code. We can always refactor that later.

By the way, I was thinking that maybe "group" is the better name, rather than "category". Mostly because it's shorter. But doesn't matter either way.

[Thrameos]

I think what you are looking for is.

        # This could be moved out of the way
        # Decorate the class to act like a dictionary
        @_jcustomizer.JImplementationFor('com.comsol.model.Model')
        class _J_comsol_model:
            def __jclass_init__(cls):
                cls._categories = {
                    'results': cls.result
                }

            def __getitem__(self, item):
                try:
                    return self._categories[item](self)
                except KeyError:
                    logger.error('This category has not been implemented yet.')

Please note the input to the _jclass_init method is a class and not the instance. After all it is a class initializer not a constructor. Thus the best you can do it return a method which you can then call.

[john-hen]

@Thrameos That was my first thought, but the JPype documentation of @JImplementationFor convinced me otherwise, as it uses the signature def __jclass_init__(self) in the example for java.util.Map. Only a very close look at the JPype source code and then the Python documentation of Mapping.register reveals that self here is indeed a class, not an instance.

[max3-2]

So what’s the take ? Should we get this customization to work an hide the getters for the groups in that way or do you prefer the model class? The former tends to be a bit implicit but the code could be an an arbitrary place and is easily expandable (if it works), the latter clutters the model class, especially the init but is fairly straight forward. Or a complete different approach?

[john-hen]

I prefer the straightforward approach. Because it's, well, straightforward. Makes the code easier to understand, which is always a good thing. Like I said, we can refactor once it works and if the code duplication gets out of hand.

[max3-2]

FWIW I tested @Thrameos approach and it does work indeed. After the customization, one could call model.java['results'].

• Still, we need to dive into the java object. This could be avoided by shadowing the java dict in the model init, which would make the code more obscure.
• The customization is not too clear for someone not into the project.

I think, as you said, it would be better to keep this idea and stash it for something else. Thus, my suggested approach:

1) Add a dict to the model init to keep track of groups by some clear names. While the dict then clutters the init a bit, we do not need any tricks like the customization or the lambdas. Should this be a private with a getter? Should this use a __getitem()__ to use dict notation (which would be a little confusing since __setitem()__ is useless here) 2) Add some base functionalities discussed above

[john-hen]

Sounds good. I think it should be an internal dictionary _groups created in __init__(). The keys are the group names, the values the corresponding Java objects. Then we can have a public method groups() that returns the keys/names, and _group() as an internal access method. This would preserve some flexibility down the line, in case we also want to have "dynamic" groups, like physics interfaces or plot groups, depending on what's defined in the specific model.

[max3-2]

I built a basic implementation in https://github.com/max3-2/MPh/tree/m32-model-features

This seems to work for some basic cases I tested however this is very alpha. If you find the time please take a look and let me know what you think and what needs adaption. I still find the names and the logic not perfect. If you want to move this into a review state I can create the PR

[john-hen]

Looks promising. Hold off on the PR for now, we can do that when this is all done. I'll have to rebase it on master anyway, and will certainly push more commits until then. It's easy enough for me to follow along by going from top to bottom in model.py, as I can (still) tell what's new and what's already been there. And yes, let's stick to this basic case. Once it works for functions and results, it will be easy to expand to other groups.

Using plurals for the group names is a good idea, I like that. Makes more sense to me as well.

I generally avoid using the @property decorator. I feel it offers little benefit over an access method. And in Python, setter and getter can always be the same method anyway, like parameter() does it for example, thanks to optional arguments. This way, I don't have to remember every time if I need to put parentheses after the name or not.

I don't think we need get_group(). It does the same as _group(), and we don't have to to return Java objects publicly.

I'm not worried that "property" is a built-in. The same is true for "type". As long as these names are used inside a local name space (class method, function argument), I don't see a problem with it. I still think "property" is the better name, as it maps to the same thing in the Java API, and that's what we're going for (in this case). Sure, "setting" is used in the GUI, so there's an argument for that too. But I think "property" is more precise.

That access method for the property/setting needs a little more work. It should return the value if called without a value, and not return anything if called to set a value. This is the same getter/setter idiom as mentioned above. Right now, it returns values in both cases. And I'd just go with a single value, not a dictionary, i.e. drop **kwargs in favor of value=None. It makes your life easier when you code it, and I don't think users absolutely need to set multiple properties all on the same line of code. More importantly, we want to use java.getValueType(name) here and switch cases depending. Much like mph.inspect() does it.

If _feature() could indeed replace _dataset() and _solution(), that would be great. I don't quite see it yet. _dataset() also handles the case of getting the default dataset. But if it's possible, by all means. The method could also be named _node(), by the way, seeing as feature can mean so many things already.

Wherever you use "groupname", it should just be "group". As far as the public API is concerned, it's implied that this is a name. Generally, I think, I've stuck to the convention of using "name" for the argument if "name of what?" is already answered by the name of the method itself or obvious from the first line of the doc-string. So it's remove(group, name) as it "removes the named feature from the group". Arguably, the signature could also be remove(group, feature) in this example. But it never needs to be "featurename".

What I'd recommend doing at this point, now that the path to follow is clear, is write test cases. Like, add functions test_create(), test_property(), and test_remove() to test_model.py. Having tests helps us judge if we like the API design, on top of serving its actual purpose, to test the implementation. Ideally, the tests would work with the existing model capacitor.mph. If not, we can extend it. Though if we modify the test model, let's make sure we use Comsol 5.5, not 5.6, so tests can still be run with two different versions.

[max3-2]

Thanks for the fast review. Here are my thoughts, if I left something out I have most likely adapted the code to match your suggestion.

I generally avoid using the @property decorator ...

All right. However in this case there will not be a setter I think

I don't think we need get_group(). It does the same as _group(), and we don't have to to return Java objects publicly.

Not exactly - it returns the java object and not the method. However I do see it not being too useful thus I removed it.

Arguably, the signature could also be remove(group, feature) in this example.

Since feature is used for the COMSOL type string of the node, I would keep name here since this is the reference supplied by the user. Or we name that type and then have `feature free

That access method for the property/setting needs a little more work...

I changed some. Most noticeable is that I created a new private function in tools to typecast when we need that at different places. I think it should be possible to change the _typecast_porperty to work in both direction for the setter...I can take a look at that

If _feature() could indeed replace _dataset() and _solution() ...

I see some issues here and will take a look but thats not too high on the list right now.

What I'd recommend doing at this point, now that the path to follow is clear, is write test cases...

I will get to that soon :)

[john-hen]

Great. Looks like this is coming together nicely.

Since feature is used for the COMSOL type string of the node, I would keep name here since this is the reference supplied by the user. Or we name that type and then have `feature free

Yes, I think we should use type in the signature of create(), stick to Comsol's terminology as much as we can. Like I said, I don't think it's a problem that type shadows the built-in. As far as I can tell, we only need isinstance to distinguish types. And if one day we do need the built-in, we can still rename the argument. That doesn't break the API... too much (unless people call the method with type as a keyword argument).

[max3-2]

Im currently working on the typecast to COMSOL. This works well but for the case of string matrices which will most likely not work easily. I will try some more but one way around would be using the setIndex() method and either

• Iterating over the whole array
• Allowing the user to select the position (not good I think)

Also the typecasting will end up not being too pretty. I tried some automated approaches with lists but since there are a lot of callables I ended up with too many lambdas. I ended up scrapping that and chose to have a few more lines but fairly clear code..

[max3-2]

I managed to get the 2D arrays working by recreating them. I don't think memory consistency / weak references are of importance here. I also added all the features we discussed and worked some more on naming and small docstrings.

I added a test for the type conversion which I will adapt to the current test style. While this is not perfect and not pulling all the methods from mph, it can serve as a quick test when there are changes in either numpy or jpype.

I will work out some tests as discussed above

[max3-2]

Added some basic testing. Please let me know what you think and where we need some more adaptions

[john-hen]

I'll need some time to look at all of it, and think about it, since it's quite a big change, so will have to get back to you later, probably the end of the week. But now it's the complete package with tests and all (thanks!), so could you create the PR now please? I'll need a separate branch for testing anyway.

[john-hen]

So I merged the PR into master and added tests for all data types that are currently being used in node properties of the demo model. That is to say, there may be more data types, ones that I'm not aware of, but this should cover the bulk of them.

On that note, I've also changed the return type for Java StringArray and StringMatrix to regular Python lists, as opposed to NumPy string arrays before. So maybe the conversion in the other direction (from NumPy string or object arrays) is also no longer needed. As you pointed out, type-casting of lists and tuples is handled by JPype on the fly, so it's best to just rely on that. And the syntax on the Python side is cleaner with lists/tuples anyway.

I'm still thinking how to support access to properties on all levels of the model tree/hierarchy: either search the tree and act on first node found with given name, or use the hierarchy notation and traverse the tree according to that. But there's currently not even an example of such a case (node on third level) in the demo model, so that needs to come first to get a feel for what this would look like in code.

[max3-2]

Thanks for all the work! If I understand this rights you currently opted for the additional method to set fields?

I have done some work and you can take a look at https://github.com/John-Hennig/MPh/compare/master...max3-2:m32-thirdLevelExample?expand=1. This is obviously WiP (or a suggestion what I had in mind with this approach) and breaks the tests. I did the following:

• Added a traverse method to get nodes using the discussed hierarchy notation
• Edited the methods relying on node to adapt to the new method
• Edited model.features to be generalized using the new traverse method
• Edited capacitor.mph and added a thermal study which shows a deeper level using a thin layer. This includes a new study and two new plot groups showing the results. I couldn't find an appropriate electrical model however there should be something like resistance I assume.

This has some advantages:

• Diving into the model tree to edit properties, remove nodes and print features. Nothing else - this keeps it fairly save and easy to understand.
• Printing features of arbitrary levels (This can internally replace all the inspection methods, e.g. a call to model.features('rootgroup') is the same as model.rootgroup()
• Should fix #33
• Fairly clear errors since each level is subsequently checked.
• Small code overhead

So editing a very deep layer would be model.property('physics->thermal->anode insulation->anode ext temp', 'T0', '600 [K]')

One could remove elements using any level (but root), e.g. model.remove('physics->thermal')or model.remove('physics->thermal->anode temp')

Currently, model.toggle() is not adapted yet but could be changed too (In general, one could define multiple BCs under a node like thin layer....). This would however change the API from a released version. To some extend, one could define methodwrapper(*args) and select the method by the number of args and raise a deprecation warning. Currently, I only see model.toggle() and model.remove() being candidates (where the latter has issues anyway and is not released to pypi yet)

[max3-2]

And just another comment regarding

On that note, I've also changed the return type for Java StringArray and StringMatrix to regular Python lists, as opposed to NumPy string arrays before.

This is easier, thanks. However in case of matrices it should be noted somewhere (in the docs?) what is considered row and what is considered column since this is important on the COMSOL side, e.g. inner lists are rows

[john-hen]

If I understand this rights you currently opted for the additional method to set fields?

No, I didn't opt for anything, I just ran out of time yesterday. Consider everything that is now in master as work in progress. In retrospect I should have created a feature branch before we started this so you could have created the PRs against a separate branch. I'd very much like to make apply_interpolation() redundant by extending the scope of property(), and would like to harmonize the signature of other methods as well, such as toggle(), as you mentioned.

I have done some work and you can take a look [...]

That looks pretty good. Meanwhile, I have uploaded a demo script with my own ideas to the sandbox folder. It's not as elegant as your solution, in that there are more new lines of code, and I'm also not sure how robust it is. But it additionally covers feature creation like the Java example in the Demonstration chapter. I decided to use tuples instead of ->-separated strings, but that's a technicality, as it's just a string split away. We should discuss pros and cons of either approach before finalizing the release. I'll take a look at the thin-layer example sometime soon.

This can internally replace all the inspection methods, e.g. a call to model.features('rootgroup') is the same as model.rootgroup()

My thoughts exactly.

Currently, model.toggle() is not adapted yet but could be changed too [...] This would however change the API from a released version.

Ditto. toggle() is fairly new, so I could even tolerate some API breakage if it's too much trouble to implement the deprecation warning or replicate the old behavior depending on input arguments.

It should be noted somewhere (in the docs?) what is considered row and what is considered column since this is important on the COMSOL side, e.g. inner lists are rows

I think that's the normal convention for matrix order and the Comsol documentation should be sufficient. More importantly, if you don't see a use case for the Python-to-Java conversion of string matrices, then I'd rather remove it to keep things simple in the code base. We can always put it back in when there's demand. The tests currently only use lists.

[john-hen]

Some things I've learned/pondered while working on the demo script...

• I went back to using singulars for the group names. That's because I focused on creating features. And then it just reads more nicely when you do model.create('geometry'). So I figured there's some sense to it. But maybe plurals would be more readable in other parts of the API. Not sure.

• I used Java objects such as model.java.geom() instead of callables like model.java.geom (without the parentheses) when returning the top-level groups. That's because I realized that we don't have to use something like model.java.geom('geom1'), i.e., don't necessarily need to pass an argument. I could be wrong. But we can get the same result with model.java.geom().get('geom1') (where "geom1" is the tag). Using the objects everywhere just makes it simpler to traverse the model tree. Or at least that's where I landed after trying different things.

• I had to use hasattr in parts of the code. I don't like that. Rather than relying on Python introspection, I'd prefer to stick with the existing Java methods when traversing the model tree. But it seems to me that top-level groups are containers in and of themselves, so they have .tags() defined and can add child nodes via .create(), whereas lower-level groups create the child nodes inside a sub-container called .feature(). I don't know if that's strictly true, so I used hasattr to check for the existence of .feature no matter where in the hierarchy. This may not be correct/robust.

• There's also a method called .getContainer() that seems to be available on every level of the hierarchy. I didn't use it, as the search is top-down and therefore doesn't need it. But if it ever makes sense to search the model tree bottom-up, that method might come in handy.

• The Comsol API creates feature names (as opposed to tags) automatically when we don't specify anything. So a new geometry would get a name/label like "Geometry 1". I wanted to leverage that so that users don't have to specify a name unless they want to. For example, we could create a new geometry by either calling model.create('geometry', 3), which auto-generates the name, or call model.create(('geometry', 'carbon nanotube'), 3) to assign a custom name inside the parent group. The create method returns the new node path, so the auto-generated name won't get lost. Also, I would have preferred the name as a separate but optional argument (name=None), but that's not possible if arbitrary unnamed arguments (*arguments) are to follow.

• When passing *arguments to create(), we should type-cast them from Python to Java/JPype. So, as you suggested previously, it would make sense to factor out getter and setter from java.property(). I'm thinking to have functions like python_to_java(value) (which would inspect the Python type) and java_to_python(value, datatype) (which would rely on datatype being .getValueType() from the Java API). model.property() would then call one or the other depending on whether it received a value or not. And model.create() could profit from using the first one.

[max3-2]

Just some comments on your bullets and your example script. In general, this looks fairly similar with, as you said, some technical differences. Especially the remove method comes out quite similar.

In general, I think both ways of specifying the node levels are good ways to do it. My (slightly biased) opinion is that the string notation turns out more legible, is typed a bit quicker and is better with using plural names, e.g. model.create('geometries->big block') reads good and then matches the rest of the API where I think model.create('geometry', 'big block') is better but doesn't fit so well with the rest. However, tuples should be more safe (I do see fewer possible ways of misinterpreting the input) and allow group names with a -> (while I don't see anyone wanting to use that....). Also, long node levels can be hard to line break using the string notation. Another big plus of tuples is the use of returned names - if they would be returned as string notation using them would need some thought, since (copying from your example, if I get this right...)

geometry = model.create('geometry', 3)
block = model.create(geometry + '->big block', 'Block')

is not really easy to understand. Another option would be to overload the str class so that + translates to -> which is slightly implicit....I think there should be a better way. On that account, maybe we can combine both approaches by actually building a node path class that prints like string notation and writes either way, keeping tuples and maybe some additional data internally.

Bullet 2

If get() does just that, I think its the cleaner approach. The notation of model.groups('group')() is not really nice.

Bullet 3

I realized that too. However my findings were that only on top level the exception is needed, then everything is in features. Thus I used the string notation and the length of the path to distinguish this - which did work ok in my simple testing.

Bullet 4

I used that in model.remove() (and only there) with sort of the same reasoning as the bullet above.

Bullet 5

I do like the auto-naming since it complies with the process when using the GUI and the return of the node path makes it very usable and legible. One could always get the name later on if needed. I guess the *arguments are needed for more complex calls to create and are the only good way to do it. I think when committing to the hierarchy notation, a name kwarg is confusing and I think the current solution is better. We could add a logger.info('Auto name {name} assigned') for clarity.

[john-hen]

[...] maybe we can combine both approaches by actually building a node path class that prints like string notation and writes either way, keeping tuples and maybe some additional data internally

Yes, I think (now) that's where we're headed. Originally I went with tuples to avoid the dedicated node-path class, and because it allows for easier splitting/concatenation than strings. (Well, sort of.) But the more I think about it and now that I read your arguments for the string notation, the more convinced I am that a special class to handle all the node-path business makes sense.

The analogy here is pathlib.Path. We shoud have nodepath.Node or something and use it in a similar way. Including string conversion to and from. So when receiving node as an argument, we'd coerce immediately to Node, which would allow strings to be passed in. Node would also define __str__ so that it has a nice representation. It would have convenience methods or attributes like .parent, just like Path, and we could support concatenation with the / operator too.

That's because a file-system path and a node path in the model tree are kind of the same thing. There are some differences though. For one thing, the file system just exists as an external resource, whereas the Node class would have to be made aware of which model it is actually acting on. And it would have to return the Java object somehow, which also has no equivalent. We don't have "relative" paths as there's no notion of a working directory. And probably other things I'm forgetting right now.

But this sounds really cool and much like the best of both worlds. Let's do it!

[john-hen]

I used that in model.remove() (and only there) [...]

I just saw that I also used .getContainer() in model.remove() and then forgot about it. Anyway, we'll see if we'll need it to get the parent group of a node or if there's a better way.

[john-hen]

Edited capacitor.mph and added a thermal study which shows a deeper level using a thin layer.

I just downloaded your version of the demo model to take a look. But it uses the Heat Transfer Module, that is to say, not just core Comsol (whatever it's called when you don't buy additional modules). The demo model should only use core functionality, so we'll have to think of another example. Maybe just a mesh sequence with a size node under a mesh feature. Once feature creation works though, we can just leverage that in testing and don't even have to modify the demo model.

[max3-2]

the demo model should only use core functionality,

Since we get anything, I am not really aware of such limitations. But no problem to find another way around for the release. Can you access HT to test the model like this?

But this sounds really cool and much like the best of both worlds. Let's do it!

I started a Proof of concept on the branch linked above. Feel free to comment and intervene. Right now there's a some chaos and obviously stuff that will be removed but it seems to be working in the right direction

[john-hen]

Can you access HT to test the model like this?

I could, but I specifically only installed Comsol core on my dev machine just to catch this kinda thing.

Feel free to comment and intervene.

I took a quick glance, so off the top of my head...

1. What's still missing is the implementation of __truediv__(self, other) in the Node class. Much like the pathlib.Path implementation, only (hopefully) much less complicated.

2. I don't think we need to accept tuples when constructing Node. Just other Node instances and string. Tuples will certainly be the way to go for the internal representation, but we don't need to expose them.

3. We could take some more cues from Path. Like having .is_group() maybe (inspired by Path.is_dir()), and in that vein, have .is_root() or whatever make sense.

4. I think Node should have no side effects on the model. This is different from Path, which can create and delete files for example. But I think it makes sense to establish that rule, to only change the model in one place and have Node be something that is read-only. Which means new nodes should be created by model.create() and not within the Node class.

5. I think (now) the signature of create should just be create(node, *arguments) instead of create(group, *arguments, name=None). The latter is good because it makes it clear that the name is optional. But I'm hoping we can just have arguments named "node" in the Model class, but not "group". It seems more systematic to me. Just a hunch though.

6. I will have no type annotations in my code base, but you do you. :wink:

[max3-2]

I will take a look at the aforementioned points...right away regarding

Like having .is_group() and I think (now) the signature of create should just be ...

I currently have no good idea how to identify a group. Mainly, because anything is a group (root is too but with tags instead of features) - I would not add an explicit method to get that then. This leads to an issue with create when the user is allowed to either specify the final node name or let COMSOL make that up because there's some ambiguity. My solution was to catch that with having name as kwarg.

I will have no type annotations in my code base, but you do you.

Never understood them either, just use them rarely when I need introspection during debugging. I am very happy if there are none in the final code

[max3-2]

I made another large update. Most features seem to work, I tried to adapt to your bullets above. However testing is still outstanding.

One rather big outstanding point is that truediv and remove can result in nodes that do not exist in java. The former due to the implementation, the latter due to removing stuff from namespace via delete is not really nice. Currently, it is not supported by model.create() to supply nodes - mainly due to the issue with groups mentioned above. Right now Im thinking about a good way to pass a final node path (which includes the node name) to model.create() which would also comply with your 5 above.

[john-hen]

I think it's perfectly fine if nodes don't exist. In fact, that's how create would know what to do. If the node exists, it creates a feature underneath it and gets the auto-generated name. If the node does not exist, it creates a node with node.name() in node.parent().

So there's no need to routinely update the .java attribute. This should be a method that is only called when needed. (Or, in this case, it may actually be a property, to be consistent with model.java, which is an attribute.) So we only check if the node actually exists when calling exists() or when calling java().

I don't know how to tell groups apart from final nodes either. All nodes seem to be able to act as groups somehow. But then we just don't have .is_group(). Looks like we won't need it.

Seems like my newly created "rule" of only changing the model object in Model will not actually pan out. Because renaming nodes would be more naturally done in the Node class. Oh, well. Let's scrap that rule then.

[max3-2]

(Partly) never mind. Found a fairly elegant way around it. Also updated test_create.py.

I will leave it like (except minor bugfixes) this and give you some time to catch up. There will be quite some discussion to do I guess

[max3-2]

And another small notice on the merges i saw. To my knowledge, one would merge a branch creating a merge commit and then continue - in your merges I see my commits being part of master. This is perfectly fine with me, however the branch is lost. My commit messages are often not that clear (my bad) and without the branch they are hard to understand. However that is fully up to you how you want to handle that.

[max3-2]

Your comment came in just before mine...

In fact, that's how create would know what to do. If the node exists

So we would need to adapt model.create() again which would remove the strange kwarg structure. This only creates bugs when creating nodes that are available, e.g. the user forgot the node is already there. However I think this would be tolerable.

Seems like my newly created "rule" of only changing the model object in Model will not actually pan out. Because renaming nodes would be more naturally done in the Node class.

Following your initial rationale, I moved this to Model. I have no preference here however I think its more logical in Node and more consistent in Model

So there's no need to routinely update the .java attribute. This should be a method that is only called when needed. (Or, in this case, it may actually be a property, to be consistent with model.java, which is an attribute.) So we only check if the node actually exists when calling exists() or when calling java().

Since java creation and node creation are not coupled, I had it like this. However the method could be removed and this will be set explicitly in the one case of create

[john-hen]

To my knowledge, one would merge a branch creating a merge commit and then continue

There are three options and so far I've used the third one, "Rebase and merge". I could use the first one next time, "Create a merge commit", if you prefer. Rebase leads to a strictly linear commit history (as shown by git log), so I liked that better. Other projects I've seen seem to use "Squash and merge" only. Then the PR title usually becomes the commit title in master. I think that's the cleanest way. You'll have a lot fewer commits then, but if that's fine with you I'll do a squash next time.

[max3-2]

You'll have a lot fewer commits then, but if that's fine with you I'll do a squash next time.

As I said I don't care how many commits there are for me - as long as the package works great ;). You choose whatever suits you the best. For a clean history I would squash and for best traceback I would merge I guess.

[john-hen]

I took a stab at the new Node class myself as it's such a fun thing to code. I uploaded my prototype to the sandbox folder, which is now in a separate branch, also named sandbox. I'm quite happy with the API it exposes and also the implementation. I used hasattr only once and didn't need .getContainer at all. It's just a demo, so not integrated with the rest of the code base. Obviously this would have ramifications on the implementation of Model, but should make things there pretty straightforward. I left some things under construction, like type-casting and tag-name generation, as I saw you already addressed these issues in your prototype.

[max3-2]

I took a stab at the new Node class myself as it's such a fun thing to code.

Yeah it seems that this will leverage much functionality with some simple additions. I used my branch to batch work on some 150 models and it worked quite well and fast! I really like adding features using the truediv notation with node/‘feature name’ that’s so much better than using the GUI.

Regarding the code

I tried to keep modifications to model as you suggested initially, you moved some of that to Node due to the above discussion which is more logical I think. Else I think functionality is fairly comparable. Your code seems to be more clear in therms of errors and general layout though.

One thing I noticed is that your __truediv__ does not allow nodes on nodes - can this be a use case which should be implemented?

Also you reimplemented the old create notation with name=None - I tried to avoid that one as disucssed!

Way forward

Whats the best way to proceed now? I suggest

1) Some code butchering on your side to get the feature set and code base for node you prefer from either your or my codebase 2) Tests 3) ...and more testing

[john-hen]

I used my branch to batch work on some 150 models […]

That's an impressive number of models!

I really like adding features using the truediv notation […]

So do I! It's pretty sleek. If there's one syntax I really love about Python, it's f-strings first and the pathlib.Path notation a very close second.

I tried to keep modifications to model as you suggested initially, you moved some of that to Node [...]

Yes, my bad. First ideas are not always best ideas. And working hands-on with the code is not the same as just looking at it. It adds more perspective.

That being said, I still think the access to this functionality via model.create(…), model.property(…), and model.remove(…), as outlined in the tentative proposal at the start of this thread, will be the way I want to highlight in the documentation. Just so that users don't necessarily have to learn about the Node class and can stick to a syntax they are more familiar with. Given that almost every line of Java or Matlab code acting on a Comsol model starts with model.….

One thing I noticed is that your __truediv__ does not allow nodes on nodes - can this be a use case which should be implemented?

Sure. But what is the use case? I thought about this when I wrote the code and couldn't really see it. I don't remember using that ever with pathlib.Path (though I could be wrong), and wouldn't really know what to do with the second node on the right, especially since we don't have relative paths. So I opted for simplicity.

On that note, I also don't see the point of implementing __rtruediv__, which would allow for something like 'name'/node.

But if there's a rationale for either of these cases, let's hear it.

Also you reimplemented the old create notation with name=None - I tried to avoid that one as disucssed!

Well, first of all, you had a point there. So that's one thing. I do think this would be a rare scenario, and kind of on the user too if they don't keep track of which nodes they have created and which ones not. But it's true that in these rare scenarios, this would lead to behavior that is difficult to debug.

But the other thing is, this is in the Node class, not inside the Model class. The node should not "care" if it exists. It should just try to do its thing and not relegate that responsibility to its .parent. As node.create() creates a child node, that's not possible if the node does not exist to begin with. But model.create() could add a bit of logic here and decide what to do depending on whether the node exists or not. That was my reasoning. I mean, for what it's worth. The same reasoning could be applied to the model, of course, except that the Model class naturally has more of a "bird's eye view". So if this logic is to be implemented anywhere, it would be in Model, not in Node.

Way forward

I think it's best you create a PR against current master with your changes, I'll merge it (or squash it, as discussed), and then we take it from there. I have no local commits lined up at this point, I put everything into the sandbox branch. There's still lots of stuff to do before the next release though. Like adding tests, yes, but also documentation as well as some code reorganization. I'll use part of your code, so would do the other stuff afterwards.

For example, I'm still not sure what to do with the newly named java module. I like for modules to have a clear purpose. Type-casting from Python to Java seems fine, we just make assumptions that only depend on the Python datatype. But the other way around, we rely on java_node.getValueType(), which depends on the node and therefore should be part of the node module … maybe?

Plus, I've realized that we can retrieve the same node property as different data types. Like, for a "mesh size" node, you could query the property hmax, which has value type "Double" and so we'd retrieve it with .getDouble and get a floating-point number like 0.001. But it's also possible to retrieve it via .getString and get the original expression 1[mm]. Which may be useful. It's not majorly important, but puts a bit of a kink in the system. I wonder how the Comsol GUI handles that. Like, how does it know that there's another way that is not indicated by the value type?

Similarly, I wonder how the Comsol GUI handles tag creation. It's also "not really important", as your way sure works nicely and because we don't even expose the tags, so users would only see them if they opened the model and activated tag display. But Comsol seems to have an automated way to create a tag like blk1 from a feature of type Block. If that functionality is exposed somewhere in the Comsol API, and it feels like it would be, then I have yet to find it…

[max3-2]

That being said, I still think the access to this functionality via model.create(…), model.property(…), and model.remove(…),

Yes, I would make them node methods and also add access methods from the model class (they are static then, not perfect but I would still go that way)

Sure. But what is the use case?

I pretty much only see stacking multiple nodes, e.g. node / ‘subgroup’ will yield a node and subsequently node / ‘subgroup’ / ‘subsubgroup’ will fail. Don’t see a need for rdiv though.

Well, first of all, you had a point there.

After a bit of usage, I would go back and opt for the automatic detection - this makes coding easier, and maybe we can just find a way to raise a well understandable exception? Using either name or node path which is then either a group or a node seems to be a tad implicit for me.

I think it's best you create a PR against current master with your changes, I'll merge it (or squash it, as discussed), and then we take it from there.

Will do after some last checks!

For example, I'm still not sure what to do with the newly named java module.

I think the content is okay but the name doesn’t fit too well...

Plus, I've realized that we can retrieve the same node property as different data types

That is a strange one even in the GUI. IIRC you can specify the unit notation in fields even if there’s a unit in the GUI, e.g. the GUI line reads u0 = 400 [mm] m which, in my opinion trades flexibilty vs. readibility in a very bad manner. Thus I suggest to only allow the correct datatypes and stick to SI (or the model unit system) - everything else will get complicated quite fast.

Similarly, I wonder how the Comsol GUI handles tag creation.

I looked into this and didnt find anything. Currently I assume they have a hardcoded pattern list for each type of feature where they pull their blueprints. That’s what I tried to adapt by parsing the string arg if available. While this is not perfect it does distinguish the tags a bit. If thats too ugly, feel free to thrash it. I wouldnt however bother to find a cleaner way since we hide the tags anyway (there’s a small exception there thats why I added node.tag().

[john-hen]

[…] access methods from the model class (they are static then, […]

No, they're regular methods. They can't be static because they need to know the model instance.

[…] node / ‘subgroup’ / ‘subsubgroup’ will fail.

No, it won't. This already works:

>>> node = Node(model, 'functions')
>>> node
Node(//capacitor/functions)
>>> node/'step'
Node(//capacitor/functions/step)
>>> node/'step'/'subnode'
Node(//capacitor/functions/step/subnode)

I'll also add support for concatenation to the model class, so that we can do node = model/'functions'. This one will accept Node on the right so that we can transfer node locations between models.

After a bit of usage, I would go back and opt for the automatic detection - this makes coding easier, and maybe we can just find a way to raise a well understandable exception? Using either name or node path which is then either a group or a node seems to be a tad implicit for me.

I don't understand. If the detection is automatic, then we won't raise an exception. An example would help.

The way I see it, if mesh = Node(model, '/meshes/mesh') (with the demo model), then model.create(mesh/'triangular') would work when called the first time, but would fail on the second call with Operation cannot be created in this context. Whereas mesh.create('FreeTri', name='triangular') would then fail with Duplicate label X#triangular. That's the difference. The first failure is not guaranteed because there might be a scenario where the child node accepts the same arguments for creation as the parent node, but that's pretty unlikely. The second failure will occur no matter what.

Will do after some last checks!

Cool, take your time, we're in no rush.

Thus I suggest to only allow the correct datatypes and stick to SI (or the model unit system) - everything else will get complicated quite fast.

With parameters we support both (string expression and evaluated value), and I do sometimes use that because it tells me which unit the user prefers there. It's especially useful when you name a results file with a parameter value in it, then you don't want to put something like 0.00056 but rather the shorter number we'd have in more natural units. But I agree that this is not a priority and only complicates things.

I wouldnt however bother to find a cleaner way since we hide the tags anyway (there’s a small exception there thats why I added node.tag().

What's the exception? I don't mind having node.tag(), but when do we ever not hide the tag?

[max3-2]

Right away, sorry for some comments since I targeted them on my branch and have not had the time to test your detailed solutions. So the things I do not comment were false alerts. I think your implementation of create solves my concerns, I will test it as soon as possible. Also, I will create the PR next so you can pull in what you need.

What's the exception?

Some nodes need data to work, e.g Node(‘exports/data export’ , ‘Data’) will need a call to set(‚data‘, ‘a tag‘) to work, where a tag specifies a dataset. There might be others. This is what I would circumvent by aNode.tag()` method the user can get the tag without needing to dig. If this would be a single case we could catch it but I doubt that.

Another thing: I had __repr__ return the state of the java element, e.g. node.exists() what I found quite handy. Any reasoning you went against that?

[john-hen]

I just followed the example of pathlib.Path regarding the format of __repr__.

[john-hen]

This is now in main (formerly the master branch). I've bumped the version to 1.0.α as I'm sure a few more bugs will pop up in testing, which I would like to catch before the final release of 1.0.0.

[max3-2]

Again, thanks for all the work - I‘ll start testing right away tomorrow. I think we should do a pass to modernize any methods which can get easier with Node - model.toggle() comes to my mind which is just a convenient wrapper around java.set(‚active‘) which almost any Node below root has.

[john-hen]

I've already marked model.toggle() for deprecation, along with model.feature(). Both issue a warning now that the Node class should be used instead. model.load() is also deprecated. I've added model.import_() as a node-aware replacement.

We could still add more methods to Node, stuff that would be useful and rather universal in terms of a node. And add better error handling, like when a property or action is not supported by the Java node, just to avoid the ugly Java exceptions here and there. But none of that is urgent, it could come after release 1.0.

[max3-2]

I see while I was on easter break this thing exploded. I will test it as good as I can. From a quick look this seems to be extremely capable and most important, quite easy to use! Since I don’t like the GUI anyway (at least on UNIX, it feels slow and unresponsive) this will improve the use of COMSOL quite a lot! I know this is after 1.0 and needs quite some new thought, but if the evaluation works with many datasets the integration with plot tools will replace the last need of the GUI :)

I think you should think good about your answer if COMSOL gets hold of this ;)

[john-hen]

[…] if the evaluation works with many datasets […]

That's the only feature I might consider working on before 1.0. It just requires more tests, and I'm pretty sick of writing tests. It's so not fun.

I think you should think good about your answer if COMSOL gets hold of this ;)

My answer is that it's on them. Not our fault their API is stuck in like 2005. :wink: