[Feature] Implement direct getter/setter for Quantity

[FlorianDeconinck]

Description

Dear all,

We have reached this moment. Yes, after many hours of debate: I have implemented quantity[:, :, :] = 0 which will behave like quantity.data[:, :, :] = 0.

The main reason for the change is orchestration. TL;DR: when parsing, DaCe maps symbols to their name. If a buffer is used with buffer and buffer.data to access the same memory, it can confuse the parser and lead to difficult to debug numerical error.

This change is also motivated by the fact we have very little to no use for quantity.field during runtime (when we might orchestrate).

Further improvement of the API would require the use of .data and .field to fail during orchestration OR in an "orchestrate" code path. Not implemented here.

How has this been tested?

The quantity unit test has been changed to not use .data and one test has been added.

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation (e.g. add new modules to docs/docstrings/)
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules
• New check tests, if applicable, are included

[romanc]

Looks good to me - just a question about some test asserts.

[twicki]

If we have direct access without .field should we at least deprecate it here so we can remove it in the release after that?

And for my own edification: I thought we specifically put in the .field and .data functions because that was the only way we could tell orchestration to overwrite the calls for the accesses to work. Why / How did that change? Or do I misremember something here?

[FlorianDeconinck]

If we have direct access without .field should we at least deprecate it here so we can remove it in the release after that?

This doesn't really deprecate those access, what it does is make orchestration possible because those access break orchestration. Now - as I said in the PR - what is missing here is a way to forbid those access from being used in orchestrated code path. I tried a few things, it's non-trivial. This alone could warrant a deprecation and removal of the specific accessors.

The counterpoint being that .field is mostly what you want when looking in the data outside of the runtime, making it a very useful shortcut.

And for my own edification: I thought we specifically put in the .field and .data functions because that was the only way we could tell orchestration to overwrite the calls for the accesses to work. Why / How did that change? Or do I misremember something here?

No, we failed to do so. We couldn't make dace.views that worked and we kept having symbol conflicts. Those results are directly motivating this change.

[twicki]

sorry, I mistyped in my comment. I ment deprecating .data as this use-case is now covered by [] access

[FlorianDeconinck]

sorry, I mistyped in my comment. I ment deprecating .data as this use-case is now covered by [] access

Yes - that we should. We do need an access, so we'll probably (re)introduce a get_data() or get_buffer() ?

[oelbert]

1 minor correction. I do think we should deprecate quantity.data but ideally over multiple releases, and as long as we keep quantity.field.

[FlorianDeconinck]

1 minor correction. I do think we should deprecate quantity.data but ideally over multiple releases, and as long as we keep quantity.field.

The problem with keeping quantity.field is that it will not orchestrate in the runtime code. So far, I have no way to warn/cancel it when orchestration. Whereas quantity[halo:-halo-1, halo:-halo-1, -1], the field use case, will work but is ugly.

Unsolved issue.

[romanc]

Regarding deprecating .field: Quantities have a reference to the Backend, which knows if we want to orchestrate or not. Could we add a DeprecationWarning inside the .field acessor of Quantity depending on the backend? I guess that would only trigger upon usage (and not up-front), but better late than never?

[oelbert]

I don't know what to say about the field issue beyond just telling people "don't use it at runtime!"

[FlorianDeconinck]

Holding merge to figure out if we can do something clever for field. Like send a picture of Oliver with frown eyebrows with every cloned repository

[FlorianDeconinck]

Drafting: I need to go clean up all the quantity.data inside ndsl itself.

[FlorianDeconinck]

Further cleanup:

• 🧹 removed all use of .data[...] in NDSL
• 🐞 fixed reduce comm bug that was swapping buffer instead of copying. Though it is not per se a bug, this would absolutely fail orchestration
• ➕ added a dtype which seems like a fair re-exposure of the underlying memory. Kept strides and itemsize hidden, reachable via _data because those shouldn't be used directly by the user

[FlorianDeconinck]

• ➕ Restored sel API. It is niche but quite useful if you want to slice an array by dimension name without knowing it's indexing
• ➕ Added a swap_buffer methods that replaced the .data setter
• 🦾 Made hashing of the State better by actually using the __hash__ of the attributes (which means calling the custom hash of Quantity)
• 🧹 clean up the .data in the unit tests

[FlorianDeconinck]

@romanc / @twicki : lots of clean up. please re-review