heat
heat copied to clipboard
helmholtz-analytics
Replace factories.array()
Description
Replacement of factories.array with DNDarray.
Issue/s resolved: #801
Changes proposed:
- replace factories.array
Type of change
- enhancement
Due Diligence
- [ ] All split configurations tested
- [ ] Multiple dtypes tested in relevant functions
- [ ] Documentation updated (if needed)
- [ ] Updated changelog.md under the title "Pending Additions"
Does this change modify the behaviour of other functions? If so, which?
no
Codecov Report
Merging #949 (397aae8) into release/1.2.x (707a1de) will decrease coverage by
4.40%. The diff coverage is100.00%.
@@ Coverage Diff @@
## release/1.2.x #949 +/- ##
=================================================
- Coverage 95.51% 91.11% -4.41%
=================================================
Files 65 65
Lines 9976 9979 +3
=================================================
- Hits 9529 9092 -437
- Misses 447 887 +440
| Flag | Coverage Δ | |
|---|---|---|
| gpu | ? |
|
| unit | 91.11% <100.00%> (-0.01%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| heat/core/sanitation.py | 97.01% <100.00%> (+0.06%) |
:arrow_up: |
| heat/core/version.py | 75.00% <100.00%> (-12.50%) |
:arrow_down: |
| heat/optim/dp_optimizer.py | 24.19% <0.00%> (-71.89%) |
:arrow_down: |
| heat/core/stride_tricks.py | 81.53% <0.00%> (-12.31%) |
:arrow_down: |
| heat/core/devices.py | 86.66% <0.00%> (-11.12%) |
:arrow_down: |
| heat/nn/data_parallel.py | 84.13% <0.00%> (-10.35%) |
:arrow_down: |
| heat/cluster/spectral.py | 85.71% <0.00%> (-8.58%) |
:arrow_down: |
| heat/core/communication.py | 89.94% <0.00%> (-6.84%) |
:arrow_down: |
| heat/core/signal.py | 95.08% <0.00%> (-4.92%) |
:arrow_down: |
| ... and 11 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 707a1de...397aae8. Read the comment docs.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hi @Dhruv454000 , thanks for taking this on.
Make sure you check out the documentation for heat.array and the DNDarray class.
What we're trying to do here is identify those cases in which we use ht.array internally in the code, but could be calling the DNDarray class directly (because we already know or can calculate all the metadata, especially gshape).
In some cases, ht.array requires communication and synchronization among all processes (see the is_split argument) and by not using it internally unless it's absolutely necessary, we want to improve efficiency.
I hope this helps!
Hi @Dhruv454000 , thanks for taking this on.
Make sure you check out the documentation for
heat.arrayand the DNDarray class.What we're trying to do here is identify those cases in which we use
ht.arrayinternally in the code, but could be calling the DNDarray class directly (because we already know or can calculate all the metadata, especiallygshape).In some cases,
ht.arrayrequires communication and synchronization among all processes (see theis_splitargument) and by not using it internally unless it's absolutely necessary, we want to improve efficiency.I hope this helps!
Ok , I will try.
@ClaudiaComito I have checked the docs, In this module I have checked other places where fact.array was present , but they had copy=false so I didnt change them , I saw about is_split also , but didnt get any idea, If I am wrong at any part pls correct me.
@ClaudiaComito I have checked the docs, In this module I have checked other places where fact.array was present , but they had copy=false so I didnt change them , I saw about is_split also , but didnt get any idea, If I am wrong at any part pls correct me.
Thank you very much @Dhruv454000, @ClaudiaComito wanted to make you cautious when you're able to replace factories.array. If you look at the split parameter in ht.array and DNDarray, you'll see that the replacement you did here wouldn't be possible normally.
A check beforehand would be good. This way, we can be sure that we're only dealing with scalars.
A check beforehand would be good. This way, we can be sure that we're only dealing with scalars.
Done
@ClaudiaComito I have added a check to confirm it is a scalar dndarray , Is anything wrong over there?
@ClaudiaComito I have added a check to confirm it is a scalar dndarray , Is anything wrong over there?
@Dhruv454000 no nothing wrong, we can simplify things a bit (see above). When you're done with that, we can run the GPU tests and check if the test coverage is sufficient.
Also could you please update the changelog? There's a similar entry already, you can add to that one.
@ClaudiaComito I have added a check to confirm it is a scalar dndarray , Is anything wrong over there?
@Dhruv454000 no nothing wrong, we can simplify things a bit (see above). When you're done with that, we can run the GPU tests and check if the test coverage is sufficient.
Also could you please update the changelog? There's a similar entry already, you can add to that one.
Done.
Should we change that .format part with fstrings
I have no strong opinion on that, I'm hoping for the GPU tests to run through so we can optimize the test coverage if necessary
@Dhruv454000 alright, last thing now is expand the tests (test_sanitation) for the check you introduced. Here's the code that's currently not covered by tests
@Dhruv454000 alright, last thing now is expand the tests (
test_sanitation) for the check you introduced. Here's the code that's currently not covered by tests
Sure , got it.
@ClaudiaComito srry for confusion done above , now its done. Also should we add test and condition to check whether it is DNDarray or not
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}