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

get_unique_name() in node_class.R might not be unique #366

Open
njtierney opened this issue Apr 8, 2021 · 4 comments
Open

get_unique_name() in node_class.R might not be unique #366

njtierney opened this issue Apr 8, 2021 · 4 comments
Assignees
Labels
Milestone

Comments

@njtierney
Copy link
Collaborator

Perhaps this is not likely to happen, or for this to be an issue, but it seems that the rhex() function as defined isn't gauranteed to create a unique name if there are many many nodes (like 1 million).

This is used in node_class.R.

See example below.

n_rhex <- 1e6

# generate a random 8-digit hexadecimal string
rhex <- function() paste(as.raw(sample.int(256L, 4, TRUE) - 1L), collapse = "")

many_rhex <- replicate(n = n_rhex, expr = rhex(), simplify = "vector")

dplyr::n_distinct(many_rhex)
#> [1] 999874

dplyr::n_distinct(many_rhex) == n_rhex
#> [1] FALSE

Created on 2021-04-08 by the reprex package (v2.0.0)

Perhaps digest or something like https://github.com/coolbutuseless/xxhashlite could be used to give nodes unique IDs

@njtierney
Copy link
Collaborator Author

Or https://github.com/reside-ic/ids or do something like what reprex did (https://github.com/tidyverse/reprex/blob/d2996e01f045b04cd537653a39deece1025dbf35/R/aaa.R), btu this might not be unique enough.

@njtierney
Copy link
Collaborator Author

this is currently being worked on here https://github.com/njtierney/greta/tree/unique-names

@njtierney
Copy link
Collaborator Author

  • stopgap solution - create error to catch unique names of nodes - basically tell the user there is something wrong with duplicated node names and that rerunning the model should fix this.
  • create unit test to test the mechanism that generates node names and that they are unique
  • test if loading multiple greta objects from past environments results in mixed up / wrongly defined nodes/models

@njtierney njtierney added the 0.4.0 label Jul 2, 2021
@njtierney njtierney added this to the 0.4.0 milestone Jul 2, 2021
@njtierney njtierney removed the 0.4.0 label Jul 2, 2021
@njtierney njtierney modified the milestones: 0.4.0, 0.3.2 Jul 8, 2021
@njtierney njtierney modified the milestones: 0.4.0, 0.4.1 Nov 26, 2021
@njtierney njtierney removed the Up Next label Mar 30, 2022
@njtierney
Copy link
Collaborator Author

There is an issue where an error appears:

Error in distrib_constructor(tf_parameter_list, dag = self) : could not find function "distrib_constructor"

Which means it is not finding

distrib_constructor <- self$get_tf_object(distribution_node)

It makes me wonder if perhaps this is related to this issue. We have not been able to reliably develop a small reprex for this issue, so it might not be related to this one.

@njtierney njtierney modified the milestones: 0.5.0, 0.6.0 Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant