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

Progress on loadtxt and savetxt (proposition) #34

Closed
wants to merge 0 commits into from

Conversation

jvdp1
Copy link
Member

@jvdp1 jvdp1 commented Dec 22, 2019

@certik
*I modified the subroutines for sp to avoid a temporary copy of the input/output array in dp. This approach would have been aslso difficult to extent to qp.
*I added two subroutines for qp.

@ALL:
To be discussed:
*For loading, I implemented two ways: 1)copy-paste of the subroutines and changing the precision and 2)unlimited polymorphic objects. We need to decide which approach to use.
*I would be happy to extent loadtxt and savetxt to integer (1,4,8,16). Is there a way to do it automatically? Or should I repeat it manually (all subroutines will be the same, except for the type of the arrays)?
*Should we add other options, like presence of an header, max columns/rows to load,... (similar to Numpy)?

@certik
Copy link
Member

certik commented Dec 22, 2019

Thanks a lot for this work! My comments:

  1. Would you mind merging in the latest master to pick up the CI tests?

  2. Before we merge this, we should polish up the history (it seems to have my old commits that are already in master, etc.)

  3. The refactoring to number_of_columns and number_of_rows_numeric is good.

  4. Adding qloadtxt and qsavetxt is good.

  5. Regarding the savetxt_poly, that's an interesting idea. My comments are: a) do all compilers support this on all platforms? b) Will the compiler messages be clear if the user passes in incorrect / unsupported type? c) The public API should be just savetxt, not savetxt_poly.

  6. There are tools that people have mentioned that allow to generate code from a template. In something like this, when the code is literally the same, just the type changes, It might make sense to use them in this case. Then it should be trivial to also extend this for integers.

  7. I suggest you split this PR into two. In one PR do 1., 2., 3., 4. Then I can pretty much review & merge right away. In the other PR do 5. and we can discuss the various pros and cons. It might take some time to reach an agreement. In some future PR, we can also implement 6.

@certik
Copy link
Member

certik commented Dec 22, 2019

The branch is quite messed up. For some reason GitHub shows a diff against the CI file, which it should not.

First of all, you should be using a branch in your repository other than master.

Here is how you can work. First fix your remotes. Here are mine:

$ git remote -v
ondrej	git@github.com:certik/stdlib.git (fetch)
ondrej	git@github.com:certik/stdlib.git (push)
origin	https://github.com/fortran-lang/stdlib.git (fetch)
origin	https://github.com/fortran-lang/stdlib.git (push)

So do

git remote add jvdp1 git@github.com:jvdp1/stdlib.git

and if your origin does not point to https://github.com/fortran-lang/stdlib.git, do:

git remote rename origin old-origin
git remote add origin https://github.com/fortran-lang/stdlib.git

Then once your remotes are fixed, do from your current master:

git checkout -b loadtxt
git fetch origin master:master
git merge master
git push jvdp1 loadtxt

This will create a branch loadtxt from your current master, then merge the actual origin master, and finally push into your fork. Then you can create a new PR from it and things should start looking much better.

@jvdp1
Copy link
Member Author

jvdp1 commented Dec 22, 2019

@certik Thank you for the procedure. I just followed it, and here is the result:

$ git remote -v
jvdp1	https://github.com/jvdp1/stdlib.git (fetch)
jvdp1	https://github.com/jvdp1/stdlib.git (push)
origin	https://github.com/fortran-lang/stdlib.git (fetch)
origin	https://github.com/fortran-lang/stdlib.git (push)
$ git branch -a
* loadtxt
  master
  remotes/jvdp1/HEAD -> jvdp1/master
  remotes/jvdp1/io
  remotes/jvdp1/loadtxt
  remotes/jvdp1/master
  remotes/origin/master

Should I close this PR, and create a new one from loadtxt?

@certik
Copy link
Member

certik commented Dec 22, 2019

Keep this one open for now. But create a new one from the loadtxt branch.

@jvdp1
Copy link
Member Author

jvdp1 commented Dec 22, 2019

Keep this one open for now. But create a new one from the loadtxt branch.

I am afraid that if I do a PR from loadtxt, it will still have all the commits.

@certik
Copy link
Member

certik commented Dec 22, 2019

If you want, I can push a branch that's polished and you can then pull it and create a PR from it.

@jvdp1
Copy link
Member Author

jvdp1 commented Dec 22, 2019

If you want, I can push a branch that's polished and you can then pull it and create a PR from it.

Ok. Let 's do that.

@certik
Copy link
Member

certik commented Dec 22, 2019

There you go:

git fetch https://github.com/certik/stdlib loadtxt2:loadtxt2
git checkout loadtxt2
git push jvdp1 loadtxt2

I used your name and email to create the commit.

@jvdp1
Copy link
Member Author

jvdp1 commented Dec 22, 2019

There you go:

git fetch https://github.com/certik/stdlib loadtxt2:loadtxt2
git checkout loadtxt2
git push jvdp1 loadtxt2

Thanks @certik for your help. Not sure what you did but it worked. See the new PR.

@certik
Copy link
Member

certik commented Dec 22, 2019

All I've done is to take your branch (master), put it into a new branch loadtxt2, and then I merged with "master" (the latest official master) and then I did git reset master, which squashed all the commits, and then git commit -a --author="..." and that's it.

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