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

adding atan #973

Merged
merged 9 commits into from
May 1, 2020
Merged

adding atan #973

merged 9 commits into from
May 1, 2020

Conversation

gyouhoc
Copy link
Contributor

@gyouhoc gyouhoc commented Apr 29, 2020

Description

I add atan function in the function.py file. (issue:#972)

Fixes # (issue)

Type of change

adding arntan function

Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

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

Thanks very much @gyouhoc ! Could you

  1. Call it arctan for consistency with the other functions
  2. Add a test like this one
  3. Add a line to CHANGELOG.md (new feature) recording the change

@valentinsulzer
Copy link
Member

Thanks for the update - for the test, what I meant was open the file "tests/unit/test_expression_tree/test_functions.py", then add a new function under test_arcsinh which does exactly the same thing but for arctan. Then run that test_functions python script to make sure the test you have added works. Test files should be kept separate from the source code (i.e. you don't need to add the test_functions.py file to pybamm/expression_tree, you can just add a single function to the existing test file)

@gyouhoc
Copy link
Contributor Author

gyouhoc commented Apr 29, 2020

Do I run this test file from the prompt?

python test_fucntion.py gives me following error.

image

@valentinsulzer
Copy link
Member

You can just run it the way you would run any python file (either from the prompt or through your IDE) - maybe you need to activate your virtual environment?

In the test you added, you should change from arcsinh to arctan (otherwise it is just a repeat of the test above it).
Also make sure that the style tests pass, you can check this locally by installing flake8 (pip install flake8) then running flake8 from command line - for example, non-empty lines raises an error, which is why this PR says "some checks were not successful"

@gyouhoc
Copy link
Contributor Author

gyouhoc commented Apr 29, 2020

I did flake8 and conduct the test. I have following errors.

image

@valentinsulzer
Copy link
Member

Make sure you have merged the latest version of the "develop" branch

@gyouhoc
Copy link
Contributor Author

gyouhoc commented Apr 30, 2020

HI Tino,

Would you let me know how to merge the latest version of the "develop" branch?
I cloned my branch to my computer. I am a newbie to GitHub.

@valentinsulzer
Copy link
Member

Assuming you have your fork as origin and the pybamm repository as upstream, the basic command is git fetch upstream develop: see this blog post

But I downloaded you fork and tried running the test and it works fine for me, so I don't know why you're getting that error

@gyouhoc
Copy link
Contributor Author

gyouhoc commented May 1, 2020

can we use your test result for this change?

@valentinsulzer
Copy link
Member

Yes, just replace all occurrences of arcsinh with arctan in test_arctan, and remove the blank spaces from lines 144 and 160 of test_functions.py (can you see the error message here?), then we can merge it

@codecov
Copy link

codecov bot commented May 1, 2020

Codecov Report

Merging #973 into develop will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #973      +/-   ##
===========================================
+ Coverage    97.36%   97.48%   +0.11%     
===========================================
  Files          222      222              
  Lines        11689    11798     +109     
===========================================
+ Hits         11381    11501     +120     
+ Misses         308      297      -11     
Impacted Files Coverage Δ
pybamm/expression_tree/functions.py 100.00% <100.00%> (ø)
...ybamm/models/submodels/interface/kinetics/tafel.py 90.00% <0.00%> (-10.00%) ⬇️
pybamm/models/submodels/base_submodel.py 91.80% <0.00%> (-0.85%) ⬇️
...lyte_conductivity/base_electrolyte_conductivity.py 98.48% <0.00%> (-0.03%) ⬇️
pybamm/parameters/thermal_parameters.py 100.00% <0.00%> (ø)
pybamm/models/submodels/thermal/lumped.py 100.00% <0.00%> (ø)
pybamm/models/submodels/thermal/x_full.py 100.00% <0.00%> (ø)
pybamm/models/submodels/electrode/ohm/full_ohm.py 100.00% <0.00%> (ø)
...ybamm/models/full_battery_models/lead_acid/full.py 100.00% <0.00%> (ø)
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6852e0e...9eb70c1. Read the comment docs.

Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

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

Merging, thanks @gyouhoc !

@valentinsulzer valentinsulzer merged commit 4742a7e into pybamm-team:develop May 1, 2020
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.

2 participants