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

Remove deprecated code warnings and minor perf improvements #1436

Merged
merged 6 commits into from
Apr 6, 2021

Conversation

cryptonemo
Copy link
Collaborator

Aims to resolve #1435 and investigate #1426

@cryptonemo cryptonemo marked this pull request as ready for review March 22, 2021 13:41
parents
.into_par_iter()
.map(|parent| t_aux.column(parent).expect("failed to get parent column"))
.collect::<Vec<Column<Tree::Hasher>>>(),
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to collect into Result<Vec>> allowing you to acutally handle the error, instead of expecting

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I'm missing something, but it looks like not what I want. The return type is Result<Vec<Column<Tree::Hasher>>>, but w/o the expect, we'd be returning Vec<Result<Column<Tree::Hasher>>>.

Copy link
Contributor

Choose a reason for hiding this comment

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

parents
  .into_par_iter()
  .map(|parent| t_aux.column(parent))
  .collect::<Result<Vec<Column<Tree::Hasher>>, _>>()?

is what I have in mind, to avoid the expect

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hrm, that makes sense, I think I had simply noted the difference of return type if the expect was removed, not changing the collect type. Is there a difference between:

.collect::<Result<Vec<Column<Tree::Hasher>>, _>>()?

and

.collect::<Result<Vec<Column<Tree::Hasher>>>()?

They both appear to work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They both appear to work.

Nevermind, just seems to be the type of error, which is implied either way (explicitly or implicitly).

@cryptonemo
Copy link
Collaborator Author

Rebased against master

@cryptonemo cryptonemo merged commit 9267de6 into master Apr 6, 2021
@cryptonemo cryptonemo deleted the warnings-and-perf branch April 6, 2021 17:06
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.

Clean up deprecation warnings and measure performance in memory_handling code
2 participants