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

Add function for coordinates. #13

Merged
merged 3 commits into from
Mar 19, 2023
Merged

Conversation

benweishi
Copy link
Contributor

Hi, I add one more format.
If a=[[x1,y1],[x2,y2],...]
The new function to_coords(a, frmt) will output
{(x1,y1),(x2,y2), ...}
This format is useful for plotting data in latex.

If you feel it is useful, please merge it into your package.

@sourcery-ai
Copy link

sourcery-ai bot commented Feb 25, 2023

Sourcery Code Quality Report

❌  Merging this PR will decrease code quality in the affected files by 0.83%.

Quality metrics Before After Change
Complexity 20.66 😞 21.02 😞 0.36 👎
Method Length 146.33 😞 152.50 😞 6.17 👎
Working memory 11.89 😞 11.92 😞 0.03 👎
Quality 38.33% 😞 37.50% 😞 -0.83% 👎
Other metrics Before After Change
Lines 375 385 10
Changed files Quality Before Quality After Quality Change
array_to_latex/__init__.py 38.33% 😞 37.50% 😞 -0.83% 👎

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
array_to_latex/__init__.py _dataframetolatex 31 😞 354 ⛔ 13 😞 22.25% ⛔ Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
array_to_latex/__init__.py _numpyarraytolatex 23 😞 327 ⛔ 13 😞 27.29% 😞 Refactor to reduce nesting. Try splitting into smaller methods. Extract out complex expressions
array_to_latex/__init__.py to_ltx 8 ⭐ 116 🙂 14 😞 52.42% 🙂 Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

@josephcslater
Copy link
Owner

Very neat! Can you add documentation and examples to the README and the Jupyter notebook showing how to use it?

@benweishi
Copy link
Contributor Author

Sorry for the late reply.

I noticed that you are putting everything into function to_ltx, so I fitted the new format into function to_ltx as well.
I also slightly changed the implementation to make it more readable. You can follow my code pattern as well, prefix + delimiter.join(list_of_content) + suffix.
I also added an example to the Jupyter notebook as well.

Let me know what else I can do.

@benweishi
Copy link
Contributor Author

BTW, I'd suggest splitting the function to_ltx into three functions: to_array, to_tabular, and to_coords, because:

  • The package name already implies that the output will be some latex code.
  • to_clp is not necessary. You can give an example code in the Jupyter notebook or README to show the user how to copy a string variable into the system clipboard, just like what you are doing for the lambda function.
  • You may want to add more parameters for customizing the tabular output.
  • Some existing parameters are not necessary for to_coords function.
  • You may want to be able to output more latex formats.

What do you think?

@josephcslater
Copy link
Owner

I think these are all good ideas. I have to concede, however, that I don't even use this package myself because my job has changed. I'm happy to maintain it, but putting a lot of effort into improving it doesn't make a lot of sense for me. I'm happy to accept pull requests and consider them, though.

@josephcslater
Copy link
Owner

Do you have latex code showing how to plot these outputs?

@benweishi
Copy link
Contributor Author

benweishi commented Mar 15, 2023 via email

@josephcslater
Copy link
Owner

I meant, can you put something in the docs as an example with an additional pull request?

@dbatten5
Copy link
Contributor

dbatten5 commented Mar 18, 2023

Hi @josephcslater and @benweishi
I noticed Joseph said that he doesn't have capacity to maintain this package any more so I took it upon myself to re-write it, incorporting some of the suggestions you made on this thread. At the moment it offers just matrix and basic tabular but I'm happy to add more outputs and also consider PRs etc. The package is here https://github.com/dbatten5/arraytex if you'd like to take a look - proper documentation still be written up

@josephcslater
Copy link
Owner

Would it not have been better to just join the project and incorporate updates instead of "yet another" project? If it's the same direction, it just adds confusion to the package ecosystem.

@dbatten5
Copy link
Contributor

Well as @benweishi mentioned there was quite a lot of refactoring that could be done, as well as adding tools to automate building and publishing, a testing framework, and linting and codestyle tools on top of that. It's a lot easier to start off with that framework then add the application code on top as opposed to trying to integrate them into existing packages. I looked at the other existing packages and a lot of them haven't been updated for several years. So normally I'd agree with you but I think in this instance it was actually just a lot easier to start again

@josephcslater
Copy link
Owner

I don't disagree with refactoring, etc. Perhaps releasing it same name, but rewritten from scratch as a major release.

@dbatten5
Copy link
Contributor

Yep that's a good point, didn't think about that

@benweishi
Copy link
Contributor Author

I think @josephcslater is still actively managing this project. But do let us know when you can't, just don't let it die.
@dbatten5, if you can initialize a new branch, I can contribute some time for the refactoring.

@josephcslater
Copy link
Owner

I won't let it die. I'm just not day-to-day coding, so it take me some time. This project is far enough along that rapid development isn't needed, but I do want to get these inputs into the project.

FYE: I've maintained the Engineering Vibration Toolbox for 32 year, so... I don't walk away from my projects. (Google Vibration Toolbox)

@benweishi
Copy link
Contributor Author

benweishi commented Mar 18, 2023

I meant, can you put something in the docs as an example with an additional pull request?

In the notebook, I added some latex code of a minimal example which use the coords format. I didn't add these to the README, because I want to keep it simple.

@benweishi
Copy link
Contributor Author

I am leaving another note here for future refactoring.
It seems like the general function of this package wants to provide is that, given an array $A$ and a format function $f$, output a string in the format "{prefix} {f(A[0])} {delimiter} {f(A[1])} {delimiter} ... {f(A[n]) {suffix}". Note that the function $f$ can be another recursive call of this function.
So if we do want do the refactoring, we can start with this function, and use this to simplify all other formating.
This general function will be something like

def format(A, prefix, suffix, f)

I know format is not a good name. But I can't think a better one.
The user can also use this general function to generate their own output.

@josephcslater josephcslater merged commit b80b5d0 into josephcslater:master Mar 19, 2023
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

3 participants