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

[MISC]: Bump edition, remove late variable assignments, fix clippy lints, replace new objects with slices, and other. #43

Merged
merged 17 commits into from
Apr 11, 2022

Conversation

Uzaaft
Copy link
Contributor

@Uzaaft Uzaaft commented Mar 22, 2022

This pr does a couple of things:

  • Fix clippy listing errors
  • variable declarations for match/if chains
  • change Stringwith &str a few places(This should be done in more places, theoretically it might reduce execution times)
  • Bump edition to 2021
    @rob-p I am just wondering. Should I go ahead and bump the version number of both alevin-fry and libradicl?

@Uzaaft Uzaaft changed the title [WIP & MISC]: Bump edition, [WIP & MISC]: Bump edition, remove late variable assignments, fix clippy lints, and other. Mar 22, 2022
Cargo.toml Outdated
rand = "0.8.5"
chrono = "0.4.19"
csv = "1.1.6"
mimalloc = { version = "0.1.26", default-features = false }
mimalloc = { version = "0.1.28", default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

@Uzaaft : This is a dependency we have to keep at 0.1.26 for the time being because of a build error on old versions of Clang (that are used in bioconda, upon which we partially rely for distribution). For more details on this see this. Suggest changing this back to 0.1.26 until those upstream changes are merged in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 83e5f9b

@rob-p
Copy link
Contributor

rob-p commented Mar 22, 2022

Hi @Uzaaft,

Thanks so much for these changes. For the eventual next tagged release, yes; the version numbers for both of these should change. This is because when we push to crates.io, we need to rely on libradicl in crates rather than the local dev version — so that version will have to be bumped to accomodate a new alevin-fry version, since it has actually changed now.

@rob-p
Copy link
Contributor

rob-p commented Mar 22, 2022

By the way, @Uzaaft — these are really nice changes to make the code more idiomatic. Any idea why clippy might not have suggested these by default? Are there just not lints for these yet? Did you notice these by just going through the code by hand?

@Uzaaft
Copy link
Contributor Author

Uzaaft commented Mar 22, 2022

@rob-p I'm not sure why.
I used cargo clippy in the terminal and clion to highlight the issues. Clion showed a few issues that clippy didn't show, and vice versa.
Although, the &str part was not a clippy lint, just a few hard lessons previously learnt. xD

@Uzaaft
Copy link
Contributor Author

Uzaaft commented Mar 22, 2022

Which reminds me, Perhaps you(or I xD) should consider moving more usages of String to &str.
If this is something you'd like, I can go ahead and migrate everything I see to &str.
This should speed things up(or keep the performance at the same level).

@rob-p
Copy link
Contributor

rob-p commented Mar 22, 2022

That would be great! I guess I don't see the Clion ones because I'm using VSCode and or Doom Emacs for most of the development, so I only get the clippy lints. If you can move over the usages of String to &str that seem reasonable, then I think this PR will be ready to merge.

@Uzaaft
Copy link
Contributor Author

Uzaaft commented Mar 22, 2022

Emacs? You should try neovim 😝I'll move more usages of string to &str, and tag you when I am done

@rob-p
Copy link
Contributor

rob-p commented Mar 22, 2022

To be fair, I only use Doom Emacs with EVIL mode enabled. I can't edit text with non-vim key bindings.

@Uzaaft
Copy link
Contributor Author

Uzaaft commented Mar 26, 2022

Migrating things to &str is was not as easy as I thought, so it'll take some time.
Also, I see some usage of unsafe. For example line 349 in src/infer.rs.
Just wondering, why do you not want to check of the string is valid utf8?

@rob-p
Copy link
Contributor

rob-p commented Mar 27, 2022

That's a good question, @Uzaaft, and the documentation around that unsafe call should be more clear. The reason this is OK is because we control the the file we are ingesting, which contains only valid cell-level tags (here, biological cells, not cell as in RefCell). That is, in the input file, only strings of 2-bit encoded characters from the alphabet {A,C,G,T} are permitted / written. Thus, it should always be safe to assume the string representation is valid UTF8 without checking for that.

@Uzaaft
Copy link
Contributor Author

Uzaaft commented Apr 10, 2022

@rob-p The migration to &str was too ambitious. I'm going to leave them out for now, and perhaps come back to that during the summer months.
Added a bonus change where we avoid creation of new objects where they aren't needed, instead using slices.
I think the PR is ready to be reviewed.

@Uzaaft Uzaaft changed the title [WIP & MISC]: Bump edition, remove late variable assignments, fix clippy lints, and other. [MISC]: Bump edition, remove late variable assignments, fix clippy lints, replace new objects with slices, and other. Apr 10, 2022
Copy link
Contributor

@rob-p rob-p left a comment

Choose a reason for hiding this comment

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

Everything here looks great @Uzaaft! Thanks for this PR :), and I'm looking forward to your further suggestions and contributions.

@rob-p rob-p merged commit d6aa33e into COMBINE-lab:develop Apr 11, 2022
rob-p added a commit that referenced this pull request Apr 20, 2022
@Uzaaft
Copy link
Contributor Author

Uzaaft commented May 23, 2022

@rob-p I am not sure if this got merged correctly. Just checked the newest master commit, and somehow the changes I made isnt there now

@Uzaaft
Copy link
Contributor Author

Uzaaft commented May 23, 2022

Turns out, I merged the local repo wrong

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.

None yet

2 participants