IO related code reorganization #103

NikoOinonen · 2023-03-06T12:30:43Z

Fixes #46

Reorganizing code related to file IO so that all IO functions are in one module called io:

Rename basUtils to io
Move some functions from io to atomicUtils
Move several functions from GridUtils to io. Basically GridUtils now only functions as an interface to the C++ functions.
Deleted a couple of functions that were not used anywhere and did not seem important.
Fix all affected imports.

…unctions.

yakutovicha · 2023-03-06T13:21:59Z

@NikoOinonen, please resolve the conflicts. I will then review the PR.

yakutovicha

thanks a lot @NikoOinonen. The two main suggestions I have are:

Please always use the same coding pattern:

from ppafm import io

io.loadXYZ(..)

Please remove unnecessary comments.

yakutovicha · 2023-03-06T16:14:30Z

dev/FARFF/test_FARFF_AuxMaps.py

@@ -10,7 +10,6 @@
 #from ppafm import fieldOCL     as FFcl
 #from ppafm import RelaxOpenCL  as oclr
 #from ppafm import common       as PPU
-#from ppafm import basUtils
 #from ppafm.AFMulatorOCL_Simple  import AFMulator
 #from ppafm.GeneratorOCL_Simple2 import InverseAFMtrainer
 #from ppafm.AuxMap import AuxMaps


Please remove the unnecessary comments and sys.path.append. Same for all the occurrences below.

yakutovicha · 2023-03-06T16:15:12Z

dev/bakup/GUImorse.py

+        #io.saveVecFieldXsf( "DEBUG_FFcl", self.FF[:,:,:,:3].astype(np.float32), self.lvec )
+        #io.saveXSF( "DEBUG_FFcl_z.xsf", self.FF[:,:,:,2].astype(np.float32), self.lvec )


Suggested change

#io.saveVecFieldXsf( "DEBUG_FFcl", self.FF[:,:,:,:3].astype(np.float32), self.lvec )

#io.saveXSF( "DEBUG_FFcl_z.xsf", self.FF[:,:,:,2].astype(np.float32), self.lvec )

yakutovicha · 2023-03-06T16:16:36Z

dev/bakup/conv_rho_mini.py

-#import GridUtils as GU
-#sys.path.append("/u/25/prokoph1/unix/git/ProbeParticleModel")
-
+from ppafm.io import loadXSF, saveXSF


I would suggest the following pattern everywhere:

from ppafm import io io.saveXSF(..) # everywhere

yakutovicha · 2023-03-06T16:16:57Z

dev/bakup/conv_rho_mini.py

+#saveXSF( "FF"+namestr+"_x.xsf", Fx*PQ,       lvec1, head=head1 )
+#saveXSF( "FF"+namestr+"_y.xsf", Fy*PQ,       lvec1, head=head1 )
+#saveXSF( "FF"+namestr+"_z.xsf", Fz*PQ,       lvec1, head=head1 )


eliminate.

Suggested change

#saveXSF( "FF"+namestr+"_x.xsf", Fx*PQ, lvec1, head=head1 )

#saveXSF( "FF"+namestr+"_y.xsf", Fy*PQ, lvec1, head=head1 )

#saveXSF( "FF"+namestr+"_z.xsf", Fz*PQ, lvec1, head=head1 )

yakutovicha · 2023-03-06T16:18:17Z

dev/bakup/runOpenCLbatch.py

@@ -159,7 +156,7 @@ def evalFF_LJC( iZPP, xyzs, Zs, qs, poss, typeParams, func_runFF=FFcl.runLJC ):
        FF    = evalFFatoms_LJC( atoms, cLJs, poss, func_runFF=FFcl.runLJC )
        FEin =  FF[:,:,:,:4] + Q*FF[:,:,:,4:]
        Tff = time.clock()-t1ff;
-        #GU.saveXSF( geomFileName+'_Fin_z.xsf',  FEin[:,:,:,2], lvec );
+        # io.saveXSF( geomFileName+'_Fin_z.xsf',  FEin[:,:,:,2], lvec );


There are many commented lines everywhere in the files you updated. Just remove them, please.

plot_results.py

NikoOinonen · 2023-03-07T07:15:32Z

@yakutovicha Most of the problems are in the scripts inside dev, and it looks like most of those scripts are unmaintained and broken. In hindsight, I probably should not have touched them at all, but it was just easier to grep and fix every occurrence of basUtils and GridUtils than to be selective.

@ProkopHapala Could you check if these scripts are actually needed, and maybe delete or move them to some other branch if they are not important? So that we don't have to spent time modifying them when we do fixes like this.

ondrejkrejci · 2023-03-07T07:16:54Z

I am having a problem as this reorganization is heavily messing with my pull request, which is to be finished this week (if my son is not ill). Maybe I should wait to this one to be pushed and then I will change the NPY part of it?

NikoOinonen · 2023-03-07T07:22:57Z

@ondrejkrejci We can also merge yours first and then pull those fixes here. Whichever way you think is easiest.

ondrejkrejci · 2023-03-07T08:25:36Z

@ondrejkrejci We can also merge yours first and then pull those fixes here. Whichever way you think is easiest.

If this is quicker than me, go ahead.

yakutovicha · 2023-03-07T08:46:38Z

I am having a problem as this reorganization is heavily messing with my pull request, which is to be finished this week (if my son is not ill). Maybe I should wait to this one to be pushed and then I will change the NPY part of it?

My 2 cents on this topic.

An open pull request will have more conflicts with time. That is normal because other people are also working on the code. Therefore, resolving merge conflicts is a natural part of collaborative development. An open pull request should never be a reason for others to wait or to stop working.

yakutovicha · 2023-03-21T14:04:58Z

Blocked till #119 is resolved.

yakutovicha · 2023-03-23T15:27:21Z

@NikoOinonen, please merge the main branch into it so the tests are passing.

NikoOinonen · 2023-03-23T18:49:13Z

Merged main and all checks are passing.

yakutovicha

Thanks @NikoOinonen. LGTM.

I think it is a good step forward towards high-quality code 👍

NikoOinonen added 4 commits March 6, 2023 10:07

Renamed basUtils into io.

e951979

Moved some functions from io to atomicUtils and removed some unused f…

2542013

…unctions.

Moved several io related functions from GridUtils to io.

4efb2a3

Fixed path construction.

7c1f9cc

NikoOinonen requested review from ProkopHapala, yakutovicha and ondrejkrejci March 6, 2023 12:30

Merge branch 'main' into io-module

2ba441f

yakutovicha requested changes Mar 6, 2023

View reviewed changes

Removed old plot_results.py, which should be already gone.

a86673c

yakutovicha added the blocked label Mar 21, 2023

NikoOinonen removed the blocked label Mar 23, 2023

NikoOinonen added 2 commits March 23, 2023 13:45

Merge branch 'main' into io-module

b295472

Deleted files that should have been deleted in the previous merge.

05f9069

NikoOinonen marked this pull request as draft March 23, 2023 11:50

NikoOinonen added 2 commits March 23, 2023 13:56

Another file deletion.

f25f91b

Bunch of small fixes.

48769ee

NikoOinonen marked this pull request as ready for review March 23, 2023 13:14

NikoOinonen requested a review from yakutovicha March 23, 2023 13:14

Merge branch 'main' into io-module

cdcccbb

yakutovicha approved these changes Mar 23, 2023

View reviewed changes

NikoOinonen merged commit e039658 into main Mar 24, 2023

NikoOinonen deleted the io-module branch March 24, 2023 07:15

mondracek mentioned this pull request Mar 27, 2023

ppafm-plot-results fails, missing basUtils #126

Closed

NikoOinonen mentioned this pull request Apr 4, 2023

Rename basUtils to io also in sphinx docs. #132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IO related code reorganization #103

IO related code reorganization #103

NikoOinonen commented Mar 6, 2023

yakutovicha commented Mar 6, 2023

yakutovicha left a comment

yakutovicha Mar 6, 2023

yakutovicha Mar 6, 2023

yakutovicha Mar 6, 2023

yakutovicha Mar 6, 2023

yakutovicha Mar 6, 2023

NikoOinonen commented Mar 7, 2023

ondrejkrejci commented Mar 7, 2023

NikoOinonen commented Mar 7, 2023

ondrejkrejci commented Mar 7, 2023

yakutovicha commented Mar 7, 2023

yakutovicha commented Mar 21, 2023

yakutovicha commented Mar 23, 2023

NikoOinonen commented Mar 23, 2023

yakutovicha left a comment

		#io.saveVecFieldXsf( "DEBUG_FFcl", self.FF[:,:,:,:3].astype(np.float32), self.lvec )
		#io.saveXSF( "DEBUG_FFcl_z.xsf", self.FF[:,:,:,2].astype(np.float32), self.lvec )

	#saveXSF( "FF"+namestr+"_x.xsf", Fx*PQ, lvec1, head=head1 )
	#saveXSF( "FF"+namestr+"_y.xsf", Fy*PQ, lvec1, head=head1 )
	#saveXSF( "FF"+namestr+"_z.xsf", Fz*PQ, lvec1, head=head1 )

IO related code reorganization #103

IO related code reorganization #103

Conversation

NikoOinonen commented Mar 6, 2023

yakutovicha commented Mar 6, 2023

yakutovicha left a comment

Choose a reason for hiding this comment

yakutovicha Mar 6, 2023

Choose a reason for hiding this comment

yakutovicha Mar 6, 2023

Choose a reason for hiding this comment

yakutovicha Mar 6, 2023

Choose a reason for hiding this comment

yakutovicha Mar 6, 2023

Choose a reason for hiding this comment

yakutovicha Mar 6, 2023

Choose a reason for hiding this comment

NikoOinonen commented Mar 7, 2023

ondrejkrejci commented Mar 7, 2023

NikoOinonen commented Mar 7, 2023

ondrejkrejci commented Mar 7, 2023

yakutovicha commented Mar 7, 2023

yakutovicha commented Mar 21, 2023

yakutovicha commented Mar 23, 2023

NikoOinonen commented Mar 23, 2023

yakutovicha left a comment

Choose a reason for hiding this comment