Record_Component.make_empty() by franzpoeschel · Pull Request #538 · openPMD/openPMD-api

franzpoeschel · 2019-07-25T11:53:55Z

See this PR, this adds bindings for Python accordingly.

ax3l · 2019-07-25T16:16:12Z

Please also add tests :)

test/python/unittest/API/APITest.py

franzpoeschel · 2019-07-31T11:47:17Z

The datatype tests apparently fail for the HDF5 backend. I don't think this is a bug with the current PR.

ax3l · 2019-08-02T14:09:34Z

CMake backport issue in Spack is going to be fixed in spack/spack#12241
Just push again after the merge.

Maybe check already to Windows builds, they seem actually broken.

ax3l · 2019-08-06T14:39:19Z

@franzpoeschel ping: please rebase

test/python/unittest/API/APITest.py

franzpoeschel · 2019-08-27T16:06:19Z

It seems there is an issue in the ADIOS1 backend, the recent commit has made the checks pass. Leaving this line inside

ms["double"][SCALAR].make_empty(np.dtype("double"), 21)

will create datasets that cannot even be processed by bpls (in either version of ADIOS).

EDIT: It seems that the reason for this behavior is the fact that ADIOS is apparently case insensitive, choosing another name for the field will apparently work. I will push a commit in a minute.

ax3l · 2019-08-27T17:17:52Z

Huh, I wasn't aware of that :-o

ax3l · 2019-08-28T03:48:31Z

include/openPMD/RecordComponent.hpp

+     * @param dimensions 
+     * @return RecordComponent& 
+     */
+    RecordComponent& makeEmpty( Datatype, uint8_t dimensions );


Just an ascetic question: would it be better to have the Datatype last?
Also: doxygen does not describe both parameters (second name is missing, dt?)

I am used to putting that parameter first since that's how switchType does it, so I did that without a lot of thinking. You mean because it would align better with the templated makeEmpty? I actually like the order that it has now, since you first specify the datatype (as template or as normal parameter), thus also specifying whether you use the templated makeEmpty or the non-template, and next you specify the dimensions.

rc.makeEmpty< float >( 3u ); rc.makeEmpty( Datatype::FLOAT, 3u);

Think that's pretty straightforward this way around?

I've completed the doxygen description.

Okay, that looks legit as well.

ax3l · 2019-08-28T17:38:50Z

@franzpoeschel I fixed the one test that ran into timeouts with #558.
Please rebase this PR against the latest dev and and remove the [WIP] in the title when it's ready for review (looks like it).

ax3l · 2019-11-05T00:56:34Z

@franzpoeschel pls rebase me one more time :)

ax3l · 2020-04-02T17:08:26Z

include/openPMD/RecordComponent.hpp

+     * 
+     * @param dt The datatype of which to create an empty dataset.
+     * @param dimensions The dimensionality of the dataset.
+     * @return RecordComponent& 


Suggested change

* @return RecordComponent&

* @return A reference to this RecordComponent.

Orthogonal question for a follow-up PR about the return type:
with handles implemented, could we make the code usage safer by returning in all those places just a RecordComponent instead of a RecordComponent&? What do you think?

The intent of returning this at all is chaining methods, right? So, things such as recordComponent.makeEmpty(3).setUnitSI(3.0).
By forcing a copy there, we would also needlessly force copying the contained shared_ptrs, which we already do somewhat excessively at times, so I'm not really convinced.

I'm in general not a huge fan of how C++ does references and the problems in returning references is one of the major reasons (especially implicit copying upon auto x = <reference>), but do you see a safety issue in our case?

Yes, the idea is to chain methods.

I think copying a shared_ptr is okay if it prevents that a user can create a dangling reference (to RecordComponent). It makes the API safer and keeps the syntactic sugar of chaining.

test/python/unittest/API/APITest.py

See also openPMD#529 Add a non-template overload to RecordComponent::makeEmpty Add tests for previous commit Refactor python binding to use non-template version of makeEmpty Add python tests for make_empty Add an overload to RecordComponent.make_empty for numpy datatypes Add tests for numpy datatypes Fix datatype tests Only test fixed-sized types

franzpoeschel · 2020-10-20T09:07:58Z

I think this one is finished @ax3l ;)

ax3l

Thank you, this looks great! 🚀 ✨

ax3l self-requested a review July 25, 2019 16:15

ax3l added the frontend: Python3 label Jul 25, 2019

ax3l changed the title ~~Add Python bindings for RecordComponent::makeEmpty()~~ Record_Component::make_empty() Jul 25, 2019

ax3l changed the title ~~Record_Component::make_empty()~~ Record_Component.make_empty() Jul 25, 2019

ax3l self-assigned this Jul 25, 2019

franzpoeschel force-pushed the topic-makeEmpty-python branch 4 times, most recently from cc4495b to b59f84f Compare July 30, 2019 15:34

ax3l reviewed Jul 30, 2019

View reviewed changes

test/python/unittest/API/APITest.py Show resolved Hide resolved

test/python/unittest/API/APITest.py Outdated Show resolved Hide resolved

franzpoeschel force-pushed the topic-makeEmpty-python branch 3 times, most recently from 37157c4 to 6cf9888 Compare July 30, 2019 20:28

franzpoeschel changed the title ~~Record_Component.make_empty()~~ [WIP] Record_Component.make_empty() Jul 30, 2019

franzpoeschel force-pushed the topic-makeEmpty-python branch 2 times, most recently from c117b6c to 7c84fa6 Compare July 31, 2019 10:24

franzpoeschel force-pushed the topic-makeEmpty-python branch 2 times, most recently from 4b6ff6e to 67df907 Compare July 31, 2019 12:11

franzpoeschel force-pushed the topic-makeEmpty-python branch from 67df907 to 5e84e9d Compare August 7, 2019 10:34

franzpoeschel force-pushed the topic-makeEmpty-python branch 2 times, most recently from 68444d1 to ed938a0 Compare August 19, 2019 14:48

ax3l reviewed Aug 20, 2019

View reviewed changes

test/python/unittest/API/APITest.py Outdated Show resolved Hide resolved

franzpoeschel force-pushed the topic-makeEmpty-python branch 3 times, most recently from b43a6fb to 0a6377f Compare August 27, 2019 15:30

franzpoeschel force-pushed the topic-makeEmpty-python branch 2 times, most recently from 6d75f3e to 9ae2120 Compare August 27, 2019 16:47

ax3l reviewed Aug 28, 2019

View reviewed changes

franzpoeschel force-pushed the topic-makeEmpty-python branch from 9ae2120 to 31ea01e Compare August 28, 2019 09:08

franzpoeschel force-pushed the topic-makeEmpty-python branch from 31ea01e to b516c9d Compare August 30, 2019 09:18

franzpoeschel force-pushed the topic-makeEmpty-python branch from b516c9d to 81916d8 Compare November 7, 2019 07:25

franzpoeschel force-pushed the topic-makeEmpty-python branch 5 times, most recently from d090e50 to 9403705 Compare April 2, 2020 14:51

ax3l reviewed Apr 2, 2020

View reviewed changes

test/python/unittest/API/APITest.py Outdated Show resolved Hide resolved

ax3l changed the title ~~[WIP] Record_Component.make_empty()~~ Record_Component.make_empty() Apr 2, 2020

franzpoeschel force-pushed the topic-makeEmpty-python branch from 9403705 to 26a0255 Compare April 7, 2020 13:28

franzpoeschel added 2 commits October 19, 2020 14:17

Fix whitespaces

51d7e57

franzpoeschel force-pushed the topic-makeEmpty-python branch from 26a0255 to 51d7e57 Compare October 19, 2020 12:19

ax3l approved these changes Oct 28, 2020

View reviewed changes

ax3l added the api: new additions to the API label Oct 28, 2020

ax3l merged commit 1318fae into openPMD:dev Oct 28, 2020

ax3l mentioned this pull request Dec 17, 2020

compatibility issues of new openpmd-api openPMD/openPMD-viewer#296

Closed

franzpoeschel deleted the topic-makeEmpty-python branch January 28, 2021 13:26

	* @return RecordComponent&
	* @return A reference to this RecordComponent.

Conversation

franzpoeschel commented Jul 25, 2019

Uh oh!

ax3l commented Jul 25, 2019

Uh oh!

Uh oh!

Uh oh!

franzpoeschel commented Jul 31, 2019

Uh oh!

ax3l commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ax3l commented Aug 6, 2019

Uh oh!

Uh oh!

franzpoeschel commented Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ax3l commented Aug 27, 2019

Uh oh!

ax3l Aug 28, 2019

Choose a reason for hiding this comment

Uh oh!

franzpoeschel Apr 2, 2020 • edited by ax3l Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l commented Aug 28, 2019

Uh oh!

ax3l commented Nov 5, 2019

Uh oh!

ax3l Apr 2, 2020

Choose a reason for hiding this comment

Uh oh!

ax3l Apr 2, 2020

Choose a reason for hiding this comment

Uh oh!

franzpoeschel Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

ax3l Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

franzpoeschel commented Oct 20, 2020

Uh oh!

ax3l left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ax3l commented Aug 2, 2019 •

edited

Loading

franzpoeschel commented Aug 27, 2019 •

edited

Loading

franzpoeschel Apr 2, 2020 •

edited by ax3l

Loading

ax3l Apr 2, 2020 •

edited

Loading

ax3l Apr 8, 2020 •

edited

Loading