Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added scalar object #9019

Merged
merged 7 commits into from
Jun 18, 2024
Merged

Conversation

N-thony
Copy link
Collaborator

@N-thony N-thony commented Jun 18, 2024

@rdstern this replaces #9006. Have a look

rdstern
rdstern previously approved these changes Jun 18, 2024
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@N-thony wow, great. Looks fine now. I would really like to get this added and also document it. @Patowhiz over to you. If small changes in the code are needed, and you are happy to make them, then I would accept that, so it can be merged quickly.

Copy link
Contributor

@Patowhiz Patowhiz left a comment

Choose a reason for hiding this comment

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

@N-thony this looks good. Just a few commit suggestions and a question. Then I'll merge this.

instat/static/InstatObject/R/data_object_R6.R Outdated Show resolved Hide resolved
instat/static/InstatObject/R/data_object_R6.R Outdated Show resolved Hide resolved
Comment on lines +415 to +423
for (j in seq_along(templist)) {
if (is.list(templist[[j]]) || length(templist[[j]]) > 1) {
if (length(templist[[j]]) > 0) {
templist[[j]] <-
paste(names(templist[[j]]), " = ", templist[[j]], collapse = ", ")
} else {
next
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@N-thony are these changes required for this pull request? I couldn't understand how they relate to the scalars addition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is more general and it fits the request by @rdstern mainly for displaying list elements in this way element1 = value1, element2 = value2, and so

Copy link
Contributor

Choose a reason for hiding this comment

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

@N-thony kindly expound more. I'm not quite sure, what you mean by more general.

  1. Does it relate to how @rdstern wants all columns metadata to be displayed?
  2. Is there a comment that he wrote which you could point me to? or a sample screenshot of current and new implementation will be useful.
  3. Is this change required for the scalar feature?

If the change is not needed for the scalar feature and it affects other parts of the code base , I suggest we make this addition in a separate pull request, then we can test for any regression. If it has to be part of this pull request, because the scalar feature relies on it then I'm hope you and @rdstern have tested for regression in the potential parts that rely on this function.

Copy link
Collaborator Author

@N-thony N-thony Jun 18, 2024

Choose a reason for hiding this comment

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

@Patowhiz that was on Skype call, maybe @rdstern could comments on this and do some more tests if any regression

@rdstern
Copy link
Collaborator

rdstern commented Jun 18, 2024

@N-thony and @Patowhiz I had not realised that was possible, but I have now tried it and I like it!

image

The mode is a troblesome summary, because it may not be unique! So for the yields there are 2 values and here they are. Wow - let's keep it, if I have understood correctly and this is the code you have been discussing!

@Patowhiz
Copy link
Contributor

@rdstern, the feature you requested prompted @N-thony to make changes to a function used in other features related to column metadata. We are currently discussing whether the new presentation changes made by @N-thony are valid for those parts as well. I understand that you find them valid for Scalars, but do you also find them valid in other metadata areas that use the function?

@N-thony, since you have identified the areas that require changes, could you help by checking where else the function you amended is being used? This would guide @rdstern and me on the areas we should test. I found this approach useful to @rdstern and saves everyone's time.

@N-thony
Copy link
Collaborator Author

N-thony commented Jun 18, 2024

@rdstern, the feature you requested prompted @N-thony to make changes to a function used in other features related to column metadata. We are currently discussing whether the new presentation changes made by @N-thony are valid for those parts as well. I understand that you find them valid for Scalars, but do you also find them valid in other metadata areas that use the function?

@N-thony, since you have identified the areas that require changes, could you help by checking where else the function you amended is being used? This would guide @rdstern and me on the areas we should test. I found this approach useful to @rdstern and saves everyone's time.

@Patowhiz I don't think this concerns the column metadata, unless I'm missing something but as far as I know this is only for the dataframe metadata not column metadata.

Copy link
Contributor

@Patowhiz Patowhiz left a comment

Choose a reason for hiding this comment

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

@N-thony thanks for looking into this, I'll close this, noting also a general change that applies to data frame metadata only has been done.

@Patowhiz
Copy link
Contributor

@rdstern I've approved this with a note as explained by @N-thony. I'll merge as soon as you approve.

@Patowhiz Patowhiz merged commit af31435 into IDEMSInternational:master Jun 18, 2024
2 checks passed
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.

3 participants