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

Clarify piece representation in infer_pieces.py #17

Closed
davidmallasen opened this issue Jul 5, 2023 · 10 comments · Fixed by #23
Closed

Clarify piece representation in infer_pieces.py #17

davidmallasen opened this issue Jul 5, 2023 · 10 comments · Fixed by #23
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@davidmallasen
Copy link
Owner

The third dimension in

value = tops[0][0][0] # B

corresponds to the inverse of __PREDS_DICT. This could be made clearer.

@davidmallasen davidmallasen added enhancement New feature or request good first issue Good for newcomers labels Jul 5, 2023
@nayan2167
Copy link
Contributor

I would like to work on this issue

@davidmallasen
Copy link
Owner Author

Great @nayan2167 ! If you need some help let me know. You can open a pull request referencing this issue when you do it.

@nayan2167
Copy link
Contributor

Hi @David-davidlxl I went through infer_pieces.py infer_pieces.py
can you explain a bit more about what exactly Is problem statement?

@David-davidlxl
Copy link
Contributor

Hi @nayan2167 I'm not sure what you're asking--are you asking what "infer_pieces.py" does? Or what you need to do with clarifying the piece representation?

@nayan2167
Copy link
Contributor

Hi @David-davidlxl what do I need to do with clarifying the piece representation that's my question

@David-davidlxl
Copy link
Contributor

The issue was opened by @davidmallasen but my guess is that he wanted to have more comments in the code related to the interpretation of tops

@nayan2167
Copy link
Contributor

yes! Exactly what is tops and how it Is related to __preds_dict, that is what I am asking

@David-davidlxl
Copy link
Contributor

Lol. If I explained everything here, there'd be no point of you performing the clarification. You should work through the code to understand what exactly it is. Otherwise I'd be happy to take over the issue and open a pull request later

@nayan2167
Copy link
Contributor

so I just have to update docstring by explaining about tops and update comments

@davidmallasen
Copy link
Owner Author

davidmallasen commented Jul 10, 2023

Hello @nayan2167 @David-davidlxl . Sorry for the delay in responding.

Thanks for updating the comments, I will merge in the pull request! However, my initial idea was to clarify the actual code. Since the third dimension of those variables corresponds to the inverse of the __PREDS_DICT dictionary, I believe creating a reverse dictionary would avoid the use of "magic numbers" in the code. For example, the inverse of B would be 0, the inverse of N would be 2, etc.

Please let me know if you need further help/clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants