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

error flag in bessel functions #4172

Closed
simonp0420 opened this issue Aug 28, 2013 · 18 comments
Closed

error flag in bessel functions #4172

simonp0420 opened this issue Aug 28, 2013 · 18 comments

Comments

@simonp0420
Copy link
Contributor

beselj0, besselj1, bessely0 and bessely1 should have methods that allow complex-valued arguments. Also, a new keyword option named, e.g., "ignore_error" defaulting to false should be provided for all Bessel function routines accepting complex arguments. If true, the option directs the routine to ignore the error flag returned by the Amos routines. The current implementation ignores the error flag in all cases.

See discussion at https://groups.google.com/forum/#!topic/julia-dev/Hy5u-2n5xVw

@staticfloat
Copy link
Member

@simonp0420 How would we check the error flag from amos?

@simonp0420
Copy link
Contributor Author

@staticfloat I'm only familiar with calling the Amos routines from Fortran. One of the subroutine arguments is an integer return argument IERR. Nonzero values indicate an error condition, as defined in the leading comments to the Amos routines. I assumed that this argument's return value is available in Julia as part of the return from calling the Amos routine after it has been incorporated into the openlibm math library. Is this not the case?

@simonp0420
Copy link
Contributor Author

@staticfloat Having just read the material on ccall for the first time, and looking at the calls to the Amos routines in math.jl, it looks like it should be pretty simple. For example, here is the call to the double precision complex Bessel function of the first kind:

function _besselj(nu::Float64, z::Complex128)
    ccall((:zbesj_,openlibm_extras), Void,
          (Ptr{Float64}, Ptr{Float64}, Ptr{Float64}, Ptr{Int32}, Ptr{Int32},
           Ptr{Float64}, Ptr{Float64}, Ptr{Int32}, Ptr{Int32}),
          &real(z), &imag(z), &nu, &1, &1,
          pointer(cy,1), pointer(cy,2),
          pointer(ae,1), pointer(ae,2))
    return complex(cy[1],cy[2])
end

It looks like IERR is available in ae[2]. I would undertake making these changes myself, but I'm not competent at using Git and GitHub--I think it could take me a long time to get up to speed. Also, I'm probably not aware of the right way to flag errors in a Julian way.

@pao
Copy link
Member

pao commented Nov 22, 2013

...but I'm not competent at using Git and GitHub--I think it could take me a long time to get up to speed.

It's not so bad, and we're definitely willing to help and teach. Start at https://help.github.com/categories/54/articles, and maybe also see http://youtu.be/wnFYV3ZKtOg and https://gist.github.com/dmbates/2712118#file_julia_git_pull_request.md

@simonp0420
Copy link
Contributor Author

@pao Thanks for the encouragement! I will take a look at the resources you mentioned. But I don't want to step on @staticfloat 's toes if he would prefer to tackle this particular issue. Elliot, how do you feel about this?

@staticfloat
Copy link
Member

Be my guest! If you run into any problems just let me know, and I'll be glad to help!

@simonp0420
Copy link
Contributor Author

Thank you, sir! This will be my first foray into open source, Git, GitHub, etc. I'm sure I will need some help, but I am looking forward to the chance to give back in a small way.

@simonp0420
Copy link
Contributor Author

@staticfloat, @pao: OK, here are the first of many stupid questions to come...

  1. I've cloned Julia to my own GitHub account, then cloned this to my local machine, started a new branch, made some initial modifications to one of the Bessel functions in base/math.jl and committed these to my local repository. I'm now having a problem pushing these changes back up to my GitHub repository. I would like to push these up to (a) back them up and (b) to allow discussion and critique from the other Julia developers. I could post some details here or if there is a more appropriate place to do so please let me know. Problem solved via the command:
    git remote set-url origin https://[email protected]/simonp0420/julia.git/info/refs
  2. I've noticed what appears to be an error in documenting the airy(k,z) method (in fact, this method appears to be a strictly utility routine called by airyai() and friends that probably shouldn't be exposed to users), and in how this method calls the Amos routine ZBIRY. Where should I bring this up and provide the details in order to arrive at consensus with the development team prior to proposing changes?
  3. General question about Julia/open source development: Before embarking on changing quite a bit of code, should I consult with and obtain consensus from other developers first? If so, how? Or does that happen only after I issue a pull request? How/where does one ask questions and discuss the rationale for a particular bit of existing code? Sorry for these ignorant questions: The largest development team I've previously worked on consisted of two people, both of whom were staring at the same terminal and jointly discussing/working on the code.

Thanks,
--Peter

@staticfloat
Copy link
Member

Hey Peter, good questions all:

  1. Yes, that should work. Note that you might also see ssh style urls, which are like so: [email protected]:simonp0420/julia.git. Both work equivalently for GitHub.
  2. If you're talking about Julia code, opening another issue on this Julia tracker would be fine. If it's in the C code in Openlibm, open an issue on the Openlibm tracker. In either case, feel free to ping me.
  3. It depends on the situation. If you already have a solution in mind and just want feedback on the technical implementation, go ahead and do your work, then open a pull request. If you're not sure the particular solution or approach to take to solve a problem, opening a pull request for each problem is standard. If you're not sure whether what you're experiencing is a problem or not, opening an issue or sending an email to the mailing list are both completely acceptable.

@simonp0420
Copy link
Contributor Author

Thanks, Elliot. I created Issue #4915 to address item 2. above, and pinged you. I'll work on setting up a pull request for item 3.

@simonp0420
Copy link
Contributor Author

@pao @staticfloat I'm having a problem with Git. On my local working machine I pulled the master branch from Julialang and then re-based my branch (amos_error_checking) on my local machine, apparently successfully. When I try to push this up to my GitHib repository, I am told that this is a non-fast-forward update. Reading about this wasn't helpful; I don't understand most of the explanations and the solution commonly suggested is to rebase, which I seem to have already successfully completed. See log of my Git session at https://gist.github.com/simonp0420/7672003 Sorry iif this is not the appropriate place to raise this question--if so, please redirect me.

Thanks,
--Peter

@staticfloat
Copy link
Member

Nope, this is a fine place to ask this. This is a pretty common confusion, and the solution is discussed here, on Stack Overflow. The short version is, you need to force your push, via git push -f. The important caveat, is that -f (or --force, for the long-form version of the flag) can be dangerous, because what that says is "ignore any changes that exist between my local repository and the remote repository, and just replace the remote repository with what I have here".

Working on topic branches all but eliminates the danger, as the worst that should happen is you nuke any unshared history on your remote topic branch. However, some versions of git have a default setting that can cause problems (indeed, we ran into this recently with the main Julia git repository); when you git push, without specifying the branch you want to push, some versions of git push ALL your branches that match names available on the remote. (So if you're working on the branch myfeature, and you have some changes on master, if you git push it pushes both myfeature and master to the remote repository). This behavior is called "matching", whereas the behavior that most people find more intuitive, is safer, and will be the default in future versions of git is called "simple", and only pushes the branch that you currently are working on. I highly suggest you switch to this behavior before force-pushing, just so you can be sure you don't accidentally overwrite other branches by accident. Here's a Stack Overflow answer describing what I'm talking about, as well as instructions on how to configure git.

If all this is making you a little nervous, don't worry too much. Because you're working on your own fork and not the main Julia repository you can't screw too much up. :) Also, you can use git push -fn to simulate a force push, and only print out what would be changed before you actually push. This is a good habit just to ensure that you are doing what you think you're doing before you actually do it.

So, in summary, here's what you need to do to get this to work:

  1. Change your git config setting to "simple": git config --global push.default simple
  2. Check to see what will be changed when you force push: git push -fn
  3. Push those changes, publishing your rewritten history: git push -f

@simonp0420
Copy link
Contributor Author

@staticfloat Thanks for the excellent guidance. I had read about -f but saw so many warnings about its dire consequences that I didn't want to try it without some assurance, which you kindly and expertly provided.

I'm just about ready to issue the pull request. I've completed all the steps described at Contributing to core functionality or base libraries except for the Travis testing. I don't seem to be able to find any documentation on how to do that. Could I ask for some assistance here, one more time?

Also, another in an endless series of dumb questions :-): I used a lot of small commits, including minor tweaks and re-writing of documentation, etc. Is this going to be annoying to the reviewers to wade through? Should I somehow compress all these commits into a single, big commit?

Thanks,
--Peter

@ViralBShah
Copy link
Member

You can squash the commits.

It would be great if you could also help improve the documentation in CONTRIBUTING.md, based on your experiences.

@staticfloat
Copy link
Member

Yes, you can do this with the interactive feature of git rebase. Here's a quick tutorial on how to use git rebase -i. Essentially, you want to "rewrite" your git history such that certain commits are (as you say) "compressed" together, and rather than do it all by hand, we want to wield the power of the computer and bend the bits to our will. To do so, we first start an interactive git rebase section on the last 5 (arbitrarily chosen, use git log to see how far back you need to go to collect all of your commits) commits:

$ git rebase -i HEAD~5

This will open up text editor with 5 lines in it, each representing a commit:

pick 8866d49 Only install julia binaries into bin/
pick d3a1856 Install git binaries into $(BUILD)/libexec, instead of $(BUILD)/bin
pick 9cf902c Don't forget to create $(BUILD)/libexec when we don't already have$
pick e3a9b33 Configure git to install to libexec rather than moving the files m$
pick 0c7064a Also try to install bin/*.{dll,bat}

By default, a rebase will do nothing; each commit has the pick operation selected, which means it that commit will be used. You can do things here like remove a commit entirely (just delete the line), reword the commit message (change pick to reword) or even squash a commit into the previous commit. In our case, we're going to squash the last two commits into the third to last commit:

pick 8866d49 Only install julia binaries into bin/
pick d3a1856 Install git binaries into $(BUILD)/libexec, instead of $(BUILD)/bin
pick 9cf902c Don't forget to create $(BUILD)/libexec when we don't already have$
s e3a9b33 Configure git to install to libexec rather than moving the files m$
s 0c7064a Also try to install bin/*.{dll,bat}

Note that you can use s or squash to mean the same thing. Finally, you save that file and exit, and git does it's thing. It'll ask you to write a new commit message for the squashed commit, and then you just git push -f it back up onto your repository!

As far as Travis testing goes, don't worry about that just yet. This is something that could really help you if you are to make a lot of changes to Julia, but for your first pull request it's a bit of setup that you don't need to go through just yet. Travis is a continuous integration service that is free for opensource projects; it builds julia and runs our test suite every time we push a commit to the repository. You'd need to setup a notification (a "hook") that gets run every time you push to your own repository, which would allow you to know if your changes to julia broke something. When you make a pull request though, your code changes are automatically run through Travis because of how we have the JuliaLang repository setup, so your code will get tested regardless. Setting it up for yourself personally is more helpful when you are working on something large and want to get it working before opening up a PR.

@simonp0420
Copy link
Contributor Author

Hope this PR isn't a turkey! ;-)

@simonp0420
Copy link
Contributor Author

@staticfloat You spent a lot of time and effort in providing me with detailed guidance--I appreciate it very much.

@ViralBShah I will be glad to take a look at adding to CONTRIBUTING.md assuming I get through this alive ;-).

@simonp0420
Copy link
Contributor Author

Fixed by commit adf1ffb and commit 870dd27

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

No branches or pull requests

4 participants