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

Refactor PLV handling to be more isolated and dynamic #328

Open
matsen opened this issue Feb 23, 2021 · 4 comments
Open

Refactor PLV handling to be more isolated and dynamic #328

matsen opened this issue Feb 23, 2021 · 4 comments
Projects

Comments

@matsen
Copy link
Collaborator

matsen commented Feb 23, 2021

Right now the PLV management in GPDAG is built in. We should have this factored out into a separate class that only knows about PLVs and likelihoods.

Also, the PLVs for GP are currently numbered in a way that is not so friendly for expanding the number of nodes:

size_t GPDAG::GetPLVIndexStatic(PLVType plv_type, size_t node_count, size_t src_idx) {                                                                                                                
  switch (plv_type) {                                                                                                                                                                                 
    case PLVType::P:                                                                                                                                                                                  
      return src_idx;                                                                                                                                                                                 
    case PLVType::P_HAT:                                                                                                                                                                              
      return node_count + src_idx;                                                                                                                                                                    
    case PLVType::P_HAT_TILDE:                                                                                                                                                                        
      return 2 * node_count + src_idx;                                                                                                                                                                
    case PLVType::R_HAT:                                                                                                                                                                              
      return 3 * node_count + src_idx;                                                                                                                                                                
    case PLVType::R:                                                                                                                                                                                  
      return 4 * node_count + src_idx;                                                                                                                                                                
    case PLVType::R_TILDE:                                                                                                                                                                            
      return 5 * node_count + src_idx;                                                                                                                                                                
    default:                                                                                                                                                                                          
      Failwith("Invalid PLV index requested.");                                                                                                                                                       
  }                                                                                                                                                                                                   
}                                                                                                                                                                                                     

That shouldn't be hard to "transpose".

@matsen matsen added this to To do in fasdag via automation Feb 23, 2021
@davidrich27
Copy link
Collaborator

@matsen Should we also take this opportunity to rename the PLV enums? Since the GP paper has moved away from the tilde notation?

@matsen
Copy link
Collaborator Author

matsen commented Feb 25, 2022

@davidrich27 YES!!!

And also make them compliant with the capitalization convention.

@davidrich27 davidrich27 mentioned this issue Feb 25, 2022
4 tasks
davidrich27 added a commit that referenced this issue Feb 26, 2022
* Adds PLVHandler, a static class of methods to facilitate GPOperations accessing
PLV indices in the GPEngine.
* Renames PLV Enums to reflect current naming conventions in GP paper.

Closes #328
@davidrich27
Copy link
Collaborator

davidrich27 commented Feb 26, 2022

@matsen Do you want to transpose PLVs from (plv_idx, node_idx) to (node_idx, plv_idx)?

@matsen
Copy link
Collaborator Author

matsen commented Feb 28, 2022

That seemed like a good thing to do if we wanted to be able to expand the number of nodes later, but how we do all of this is up to you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
fasdag
To do
Development

No branches or pull requests

2 participants