Skip to content

Conversation

@codeZeilen
Copy link
Member

@codeZeilen codeZeilen commented Dec 16, 2025

Open questions:

  • To which degree do we want to handle "weird" mapping relations (e.g. data frame that contains NULL, which is not trivial to achieve in the first place)

TODOs:

  • run pipeline with the change
  • should we add a note that there were empty cells, which will be ignored in the output?

@tscheypidi
Copy link
Member

I think a note would be indeed useful.

@codeZeilen codeZeilen force-pushed the feature/empty-mapping-targets branch from 18c7892 to b24e326 Compare January 6, 2026 13:05
@codeZeilen codeZeilen force-pushed the feature/empty-mapping-targets branch from b24e326 to 81753bc Compare January 7, 2026 08:34
@codeZeilen codeZeilen marked this pull request as ready for review January 7, 2026 08:40
@codeZeilen codeZeilen marked this pull request as draft January 7, 2026 08:42
@codeZeilen
Copy link
Member Author

Something is off. I have already implemented the note but it is not in there. Will go looking for it.

@codeZeilen codeZeilen marked this pull request as ready for review January 7, 2026 08:57
@codeZeilen
Copy link
Member Author

Found the missing commit, will squash on merge to remove the weird history.

@codeZeilen codeZeilen requested review from tscheypidi and removed request for tscheypidi January 12, 2026 13:53
Copy link
Member

@tscheypidi tscheypidi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. Just as a side note: you could probably write lines 506 - 512 more compact using the dim argument in magclass objects, e.g. something like m <- m[emptyValues, dim = i, invert = TRUE]

@codeZeilen
Copy link
Member Author

looks good. Just as a side note: you could probably write lines 506 - 512 more compact using the dim argument in magclass objects, e.g. something like m <- m[emptyValues, dim = i, invert = TRUE]

Ah, perfect, I was looking for something like this, but was not aware of that!

Also: This is such a convoluted PR commit log for such a small code change 😄

@codeZeilen codeZeilen merged commit 24d1fce into pik-piam:master Jan 13, 2026
1 check passed
@codeZeilen codeZeilen deleted the feature/empty-mapping-targets branch January 13, 2026 09:44
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

Successfully merging this pull request may close these issues.

2 participants