Feedback #1
Open
Feedback #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-classroom[bot]](https://avatars.githubusercontent.com/in/68187?s=60&v=4){kind=small}
agluszak
reviewed
Nov 14, 2022
README.md
Outdated
Comment on lines
20
to
22
| - 1.1. Implement rational numbers representation. | ||
| - 1.2. Implement matrix module for given any number representation. | ||
| - 1.3. Implement basic operations on matrices. |
There was a problem hiding this comment.
Wydaje mi się, że ta część jest bardzo mało interesująca, bo istnieją np. https://nalgebra.org/, https://github.com/bitshifter/glam-rs, ... tych bibliotek jest tak dużo, że powstała biblioteka https://docs.rs/mint/latest/mint/index.html
There was a problem hiding this comment.
więc proponowałbym raczej użycie którejś z nich i skupienie się na części wizualizacyjnej
There was a problem hiding this comment.
więc proponowałbym raczej użycie którejś z nich i skupienie się na części wizualizacyjnej
* Added Rational struct with simple operations Added Rational Added LaTeXable * Updated README.md. Stages are listed. * Added tests * quick push * Updated README.md. Co-authored-by: Mieszko Grodzicki <[email protected]>
Co-authored-by: Alek Tudruj <[email protected]> Co-authored-by: Alek Tudruj <[email protected]>
Added Rust code checks for PRs Added Rust automatic code formatting
Added GH Actions
* WIP matrix ops added add added sub wip neg * No idea how to fix it * Working neg * schodki schodki * Fixed anyhow return Err(anyhow::anyhow!(...)); $\equiv$ anyhow::bail!(...) `cargo fmt` + `cargo clippy` * dirty test commit * undo: dirty test commit * Started Mul * for loops need to be trashed later * fixed clippy * fixed fmt * Added macros for rational and normal matrices * macro rules are now safe (in runtime) * refactor * refactor 2 * Update matrices.rs * default implementation for to_latex_single * remove extern crate core * review commit Co-authored-by: Mieszko Grodzicki <[email protected]>
* First version * Other binary operators, tests * Clippy * updated a lot, pozdr * cargo fmt * Add checks for invalid expressions. * clippy * Add assignments Co-authored-by: Alek Tudruj <[email protected]>
* Siup * siup siup
agluszak
reviewed
Dec 30, 2022
There was a problem hiding this comment.
Kod się miejscami ciężko czyta, przydałyby się komentarze wyjaśniające nieoczywiste rzeczy. Mam pewne wrażenie niechlujności, niektóre metody są za długie. Pod względem ściśle rustowym mam do zarzucenia przede wszystkim to, że używacie anyhow w kodzie, który wygląda na biblioteczny, więc przydałoby się, żeby isniały konkretne rodzaje błędów. Sporo mam ogólnych uwag inżynieryjnych.
Fajnie że jest dużo testów (gdyby ich nie było, to trudno byłoby stwierdzić, czy ten kod ma jakikolwiek sens, bo nie ma na tym etapie binarki do uruchomienia).
4.5/5
| } | ||
| } | ||
|
|
||
| pub fn is_valid(id: &str) -> bool { |
There was a problem hiding this comment.
nie ma potrzeby żeby ta funkcja była publiczna
src/environment.rs
Outdated
| } | ||
|
|
||
| impl Identifier { | ||
| pub fn new(id: &str) -> anyhow::Result<Self> { |
There was a problem hiding this comment.
lepiej żeby brało String, a nie referencję, bo i tak w środku ta funkcja dokonuje alokacji w obecnej wersji, więc lepiej żeby było to explicit
src/environment.rs
Outdated
| Matrix(Matrix<T>), | ||
| } | ||
|
|
||
| pub type Environment<T> = HashMap<Identifier, Type<T>>; |
There was a problem hiding this comment.
lepiej opakować to w nowy typ, bo type aliasy tylko wprowadzają szum moim zdaniem
src/matrices.rs
Outdated
| Self { data } | ||
| } | ||
|
|
||
| pub fn new_safe(data: Vec<Vec<T>>) -> Self { |
There was a problem hiding this comment.
to nie jest safe, tylko tamta druga metoda jest unsafe. Zazwyczaj robi się jedną metodę zwracającą result i drugą panikującą (ew. trzecią robiącą UB: https://doc.rust-lang.org/std/primitive.slice.html#method.get_unchecked). Ale tutaj to chyba w ogóle nie ma sensu robić dwóch metod.
src/matrices.rs
Outdated
| steps.push(format!(r"\xrightarrow{{{}}} {}", step, matrix.to_latex())); | ||
| }; | ||
|
|
||
| fn nice<T: MatrixNumber>(coef: &T) -> Option<i64> { |
src/matrices.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn sub_coeff_to_latex<T: MatrixNumber>(coef: &T) -> Option<String> { |
There was a problem hiding this comment.
niezbyt podoba mi się wciskanie definicji metod w środek innej metody
src/matrices.rs
Outdated
| where | ||
| F: Fn(&T) -> Option<T>, | ||
| { | ||
| // We do a little trick and apply `self` twice, as it is more memory efficient |
| } | ||
|
|
||
| #[macro_export] | ||
| macro_rules! rm { |
|
|
||
| pub trait LaTeXable { | ||
| fn to_latex(&self) -> String; | ||
| fn to_latex_single(&self) -> String { |
src/matrices.rs
Outdated
Comment on lines
116
to
130
| self.data.swap(i, j); | ||
| add_step( | ||
| format!(r"w_{{{}}} \leftrightarrow w_{{{}}}", i + 1, j + 1).as_str(), | ||
| &self, | ||
| ); | ||
| } | ||
|
|
||
| if !self.data[i][c].is_one() { | ||
| let d = self.data[i][c].clone(); | ||
| for j in c..cols { | ||
| self.data[i][j] = self.data[i][j].checked_div(&d).context(CONTEXT)?; | ||
| } | ||
|
|
||
| add_step( | ||
| format!(r"w_{{{}}} : {}", i + 1, d.to_latex_single()).as_str(), |
There was a problem hiding this comment.
za długa jest ta metoda i zbyt skomplikowana, przydałoby się podnieść poziom abstrakcji, podzielić to na kawałki i wprowadzić sensowne nazwy
* Started feedback resolve. * Docs + refactor * Tests + refactor * Moved echelon to another module and extracted nested functions. * Document matrix_algorithms * fmt and clippy my beloved friends * clean up * Change `corresponding_shapes_for_mul` as suggested Co-authored-by: Mieszko Grodzicki <[email protected]>
Added [NITD] description
* initial work * Started feedback resolve. * Docs + refactor * Tests + refactor * Moved echelon to another module and extracted nested functions. * Display objects from mocked env * clean * Added simple shell * Added adding new variables into env * clippy + fmt * removed useless serde * eeeeeeeee inputs need to be better managed * is_empty fix * locale update * fixed SP/PL mismatch * changed langauge to Eng * Polacy nie gęsi * moved editor to another file * enter invalid data, parse at the end * fix * Improve shell experience * editor works * fmt + clippy * fmt + clippy * parsing instead of mocking parsing * added echelon and latex * Updated Actions to include xorg-dev and all dependencies mentioned in README.md * Updated Actions to include xorg-dev - revert of previous - added xorg-dev dependency in gh workflow * bum * clippy * GuiDisplayable for primitives * New env element windows * Solved compilation problems on some devices. Checkout dependencies * nannou-org/nannou#746 * Solved compilation problems on some devices. Updated GH workflow * Added build passing between stages * Why did we even have this * Boosted egui to 0.20.1 Now depends on our egui-toast fork * egui-toast PR note * windows note * Added translations * Minor changes * MatrixNumber is ToString now (used to many times) * Added runtime language support (args) * Added translations * Updated README.md * Polish name translated (Idea) * Missing translations + Matrix view * Added missing translations * Changed Matrix view * Missing translations + Matrix view * Added missing translations * Changed Matrix view * Added values editor * Missing translations * removed unnecessary `where` * More meaningful toasts * env in matrixes * env in editor - WIP - fix 1 * minus operator temp fix * better error info toast * minor fixes * editor error label * checked_mul is checked * cli + fmt * fmt * clippy * added clap cli args * Added git hook for fmt, clippy * Added missing translation logging * Add fractal clock (don't even ask why) Co-authored-by: Mieszko Grodzicki <[email protected]>
As we use `clap` the --help will provide information about language.
* Add unary operators to expressions. * Remove the hack from editor
* Add inverse * Add inverse button * No echelon for scalars * Add inverse for scalars
* add $ * clippy * fix blinking * bottom separator was redundant * translation fix Co-authored-by: Aleksander Tudruj <[email protected]>
* LaTeX test * test 2 * test 3 * operations * GUI and Shell * examples and editor * echelon example * echelon example - fix * echelon example - fix 2 * echelon example - fix 3 * echelon example - fix 4 * echelon example - fix 5 * echelon example - fix 6 * echelon example - fix 7 * echelon example - fix 8 * echelon example - fix 9 * echelon example - fix 10 * did my best to fulfill the requirements * giiiit
* Added zoooooming * fix alignment * Add reset button
* Allow changing language via GUI * fmt + clippy * move lang select to the right * as locale changes dynamically we store the calculated translation maps in memory --------- Co-authored-by: Mieszko Grodzicki <[email protected]>
* release workflow test * fix * changed trigger event * name and ref fix * name fix again * added missing system dependencies * install dependencies on linux machines * missing dependency * nosz kolejna * zaraz mnie coś trafi * nawet nie wiem co to ten musl do kosza * losowe dependencje robią brrrrrrrrr... * another one bites the dust * it was nice meeting you. Now go away * where is my bed
* Fixed Fourier spelling error * imports sorted * more spellings fiexed * removed useless comment
* Added Matrix::transpose to the matrix library * Added Transpose button to GUI * Added missing .T translations * Cargo clippy
Bumps [enumflags2](https://github.com/meithecatte/enumflags2) from 0.7.6 to 0.7.7. - [Release notes](https://github.com/meithecatte/enumflags2/releases) - [Commits](meithecatte/enumflags2@v0.7.6...v0.7.7) --- updated-dependencies: - dependency-name: enumflags2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Project is now a library with main-like function to enable doc testing * fixed not working doc-tests * fmt
* Add functions * clippy * added expr tests in parser * Update error * added identity to parser tests * testing existance of std functions * fmt --------- Co-authored-by: Aleksander Tudruj <[email protected]>
Bumps [xml-rs](https://github.com/kornelski/xml-rs) from 0.8.4 to 0.8.14. - [Changelog](https://github.com/kornelski/xml-rs/blob/main/Changelog.md) - [Commits](kornelski/xml-rs@0.8.4...0.8.14) --- updated-dependencies: - dependency-name: xml-rs dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Added floats as field * added --approx parameter * removed code duplicates * some refactor * added unit tests for floats
* Cargo update * Cargo update * Cargo clippy+fmt * cargo update --------- Co-authored-by: Mieszko Grodzicki <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
👋! GitHub Classroom created this pull request as a place for your teacher to leave feedback on your work. It will update automatically. Don’t close or merge this pull request, unless you’re instructed to do so by your teacher.
In this pull request, your teacher can leave comments and feedback on your code. Click the Subscribe button to be notified if that happens.
Click the Files changed or Commits tab to see all of the changes pushed to
mainsince the assignment started. Your teacher can see this too.Notes for teachers
Use this PR to leave feedback. Here are some tips:
mainsince the assignment started. To leave comments on specific lines of code, put your cursor over a line of code and click the blue + (plus sign). To learn more about comments, read “Commenting on a pull request”.main. Click a commit to see specific changes.For more information about this pull request, read “Leaving assignment feedback in GitHub”.
Subscribed: @tudny @mgr0dzicki