Skip to content

Conversation

@volodia99
Copy link
Collaborator

No description provided.

@volodia99 volodia99 added the enhancement New feature or request label Nov 4, 2024
@volodia99 volodia99 changed the title [ENH]: write and read native coordinates in VTK [ENH] write and read native coordinates in VTK Nov 4, 2024
@neutrinoceros neutrinoceros changed the title [ENH] write and read native coordinates in VTK ENH: write and read native coordinates in VTK Nov 4, 2024
@neutrinoceros neutrinoceros force-pushed the vtk-native-coords branch 3 times, most recently from 79f2285 to d6960c9 Compare November 5, 2024 06:55
@neutrinoceros
Copy link
Collaborator

I rewrote the branch history to help bisect the current issue: vtk outputs are corrupted and unreadable. The breaking commit is eaa2322 (as one might expect).

I also changed back the reading logic so that

  • all "surprise" fields are processed by the while loop that populates self.data
  • only then, override reconstructed coordinate fields
    this is a much more flexible approach with respect to backward and forward compatibility, though I also expect we'll need to update the reading logic in the while loop so that it also accept fields of arbitrary size, which I'll try now.

@neutrinoceros
Copy link
Collaborator

I think a fundamental issue with our current approach is that we're writing arbitrary fields in the style of the VTK header but outside said header. We need to either:

  1. change the style
  2. actually add these fields to the header

I think option 2) is preferable but I don't know if it's allowed.

@neutrinoceros
Copy link
Collaborator

found a problem downstream: https://github.com/volodia99/nonos/pull/371#issuecomment-2459596390
Specifically, what I'm seeing is that "cell center" arrays have identical data as cell edges (the latter is correct). It may still be an issue with reading, but this needs to be clarified.

@neutrinoceros
Copy link
Collaborator

all clear, the issue was downstream

Co-authored-by: volodia99 <gaylor.wafflard@univ-grenoble-alpes.fr>
@volodia99 volodia99 marked this pull request as ready for review November 6, 2024 14:05
Co-authored-by: Clément Robert <cr52@protonmail.com>
@glesur
Copy link
Contributor

glesur commented Nov 15, 2024

Thanks for this improvement, that seems to fix the issue of reconstructing the cell coordinates from the VTK cartesian coordinates. However, I think there is a regression: the original version of vtk_io reconstructed the cell faces and stored it in xl. Despite the "l" suffix, this was really all the cell faces, i.e. including the rightmost cell face coordinate (in other words, you have one more cell face than cell centres). With a scheme (c representing the cell centres and | the faces):

|        c         |          c         |        c         |        c           |
xl(0)   x(0)     xl(1)       x(1)      xl(2)    x(2)      xl(3)    x(3)       xl(4)

This new implementation misses that last rightmost face (xl(4) on my scheme), since you actually store dataBlock::xl which is effectively the cell left face coordinates. I think we should actually try to keep this.

@glesur
Copy link
Contributor

glesur commented Nov 26, 2024

Actually, my previous comment was incorrect, it does include the last rightmost face. So we're good to go.

@glesur glesur self-assigned this Nov 26, 2024
glesur added a commit that referenced this pull request Nov 26, 2024
ENH: write and read native coordinates in VTK (#292)
@glesur glesur merged commit f2d83c7 into idefix-code:develop Nov 26, 2024
@glesur
Copy link
Contributor

glesur commented Nov 26, 2024

Manually merged as github wouldn't let me do it from their website.

This was referenced Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants