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

Importers #2

Open
szaghi opened this issue Apr 30, 2013 · 15 comments
Open

Importers #2

szaghi opened this issue Apr 30, 2013 · 15 comments

Comments

@szaghi
Copy link
Owner

szaghi commented Apr 30, 2013

Importers are missing.

@ghost ghost assigned szaghi Apr 30, 2013
@szaghi
Copy link
Owner Author

szaghi commented Jun 10, 2015

Start to merge @victorsndvg work from its fork.

There are a lot of work to do...

Victor programming style

Victor programming styles seems too much different from mine. Take this in count.

Library source size

The library source is becoming huge. Consider to exploit include for making light the source file.

Speak of this with victor.

XML Parsing

Victor XML parsing seems complex: pay attention to the auxiliary procedures he is using.

Speak of this with victor.

Library API

Victor adopted a new set of public interface where the suffix _READ means importer procedures. I was thinking to hide the difference between importers and exporters into the name with something like:

! importer
E_IO = VTK_INI_XML(...,action='load')
! export
E_IO = VTK_INI_XML(...,action='save')

Speak of this with victor.

@victorsndvg
Copy link
Collaborator

Hi Stefano,

I'm glad to see that you are reviewing the new and big piece of code... Good luck! :)

Feel free to ask or suggest anything. I will try to help you with any doubt and I can also try to adapt or reimplement the code according to your suggestions. Probably it will be a long conversation :)

Best regards,
Víctor.

@szaghi
Copy link
Owner Author

szaghi commented Jun 10, 2015

@victorsndvg

Thank you very much for your kindness, it is very appreciated.

Let's go!

Lib_VTK_IO.F90 source file size

In your version, that is the most up-to-date, the lines counting is now 12572: too much for me. I am thinking to exploiting include statement with the following logic:

the procedures of (let us say) VTK_GEO_XML will go into Lib_VTK_IO-GEO_XML.inc thus the actual implementations of the

module procedure VTK_GEO_XML_STRG_1DA_R8, VTK_GEO_XML_STRG_3DA_R8,  & ! real(R8P) StructuredGrid, 1D/3D Arrays
                   VTK_GEO_XML_STRG_1DAP_R8,VTK_GEO_XML_STRG_3DAP_R8, & ! real(R8P) StructuredGrid, 1D/3D Arrays packed API
                   VTK_GEO_XML_STRG_1DA_R4, VTK_GEO_XML_STRG_3DA_R4,  & ! real(R4P) StructuredGrid, 1D/3D Arrays
                   VTK_GEO_XML_STRG_1DAP_R4,VTK_GEO_XML_STRG_3DAP_R4, & ! real(R4P) StructuredGrid, 1D/3D Arrays packed API
                   VTK_GEO_XML_RECT_R8,                               & ! real(R8P) RectilinearGrid
                   VTK_GEO_XML_RECT_R4,                               & ! real(R4P) RectilinearGrid
                   VTK_GEO_XML_UNST_R8,VTK_GEO_XML_UNST_PACK_R4,      & ! real(R8P) UnstructuredGrid, standard and packed API
                   VTK_GEO_XML_UNST_R4,VTK_GEO_XML_UNST_PACK_R8,      & ! real(R4P) UnstructuredGrid, standard and packed API
                   VTK_GEO_XML_CLOSEP

will go into Lib_VTK_IO-GEO_XML.inc thus the procedures definitions that are now after the main module contains will be replaced with a simple

include("Lib_VTK_IO-GEO_XML.inc")

The same for all big generic interface.

What do you think about this?

@victorsndvg
Copy link
Collaborator

@szaghi

I've never used the include statement before. I have to read something about it.

My first impression, and baseless opinion, about include statement is that it seems a very old-school-F77 style. May be i'm wrong, sorry for my impudence :)

And, why not to modularize the code?

I mean, reorganize the source code in different modules. For example:

File Lib_VTK_IO-GEO_XML.f90 should contain the module Lib_VTK_IO-GEO_XML_module with all the subroutines and interfaces for both reading and writing, or only for one of them.

File Lib_VTK_IO.f90 should use all the modules and work like a front-end for the rest of modules.

What do you think ?

@szaghi
Copy link
Owner Author

szaghi commented Jun 10, 2015

@victorsndvg

You are (almost) right.

include is in standard from very old time (I am not sure but I think that f66 should have). However, it is still not included into the deprecated feature, because it has its pros:

  • using include avoid the compiling cascades hierarchy that comes with use module...; it is not a real problem, I have FoBiS.py :-) but other users may complain about;
  • using include means literally that the contents of the file are inserted where is the include statement, actually replacing the statement itself.

Indeed, my first thought was (as your) to use module for packing the interfaces. However, besides the new compiling hierarchy (that is a minor issue), my main concern is about the (ab)use of some global variables defined into the Lib_VTK_IO main module: if we use (sub)modules for packing the interfaces, we must ensure to avoid circular modules use because interface-modules must access to the global variables defined into the main module and the main module must use the interface-modules... if we agree to modularize the library, we must plan a module also for global variable (being careful with multi-thread security...).

Ok, tomorrow I will propose a possible modularization and a possible include exploiting trying to highlight pros and cons of both.

See you soon.

@victorsndvg
Copy link
Collaborator

@szaghi ,

it's nice to learn reading your posts. Thank you.

I'm agree with you, there must be some modules (or only 1) containing global variables and common procedures. As an early/basic idea, and according to this, to avoid circular dependencies between modules we could think in a 3 (or more) level hierarchy. The lowest level could contain those common and global things and the highest level work as a front-end.

The biggest work should the placed in the intermediate levels, containing the interfaces an procedures for effective reading and writing.

The highest level will use the intermediates level modules and, following the hierarchy, those levels will use the lowest level.

Do you think this is possible? Do you see any potential problem of this simple schema?

@szaghi
Copy link
Owner Author

szaghi commented Jun 11, 2015

@victorsndvg

You are completely right. This night, "sloshing" my daughter, I realized that modularization is definitely the better approach because allow the easy maintenance of the library and favorite the future refactoring (I am planning to refactor the API to allow OOP, that, besides other things, means encapsulate all...). I think to discard the idea of exploiting include facility.

Thus we can think to our modularization. I like to try implement the modularization step by step with you. The following is just a first idea.

Modularization hierarchy

Three levels of abstraction (as you suggest):

  1. back-end module, that should:
    • contain back-end auxiliary procedures (strings manipulation, xml parser, etc...);
    • contain global variables: their usage should be minimized (in the future OOP API must be completely avoided for security) and shadowed;
    • be used by procedures-implementation modules;
  2. procedures-implementation modules, that should:
    • group the actual implementation of a generic interface;
    • expose only the generic interface concerning with;
    • not use other modules of the same hierarchy level, i.e. other procedures-implementation modules;
    • use the back-end module;
    • be used by the front-end module;
  3. front-end module, that should:
  • use all the procedures-implementation modules;
  • exposing only the generic interfaces provided by the procedures-implementation;
  • not use the beck-end module.

Modules naming

No idea... do you have some suggestions?

New branch generation

Today I will create a new branch, beside the master one, named importers. I would like to allow you to directly control with me my gitrepo, thus you can control my mistakes... is it ok for you?

See you soon.

szaghi added a commit that referenced this issue Jun 11, 2015
Start library modularization as described into #2

The modules naming convention is temporary: ask victor his suggestion.
szaghi added a commit that referenced this issue Jun 11, 2015
Add (sub)modules: the library is now modularized as #2
@szaghi
Copy link
Owner Author

szaghi commented Jun 11, 2015

@victorsndvg

Just added a new branch, importers.

The modularization is done as above described.

The naming convention adopted is trivial, but feel free to suggest a more smart one.

Into the back-end module I have already inserted your auxiliary procedures. Besides some minor changes for conforming my (bad) style, I have made some major changes:

  • into your original source you call your procedures with positional arguments without using the formal name: for improving safety (check at compile time) I have passed dummy arguments with formal name association, e.g. https://github.com/szaghi/Lib_VTK_IO/blob/importers/src/Lib_VTK_IO_Back_End.f90#L440
  • I have changed the order of optional dummy arguments into your procedures: indeed I am not completely sure about this (please check my following statement with your manuals if possible), but I remember that when you define a procedure having some optional arguments these should come before the non optional ones in order to achieve disambiguous signature when you call that procedure with only positional arguments (without explicit formal name) the compiler is able to correctly associate dummy arguments with formal ones; however, my memory can be wrong and maybe the disambiguous signature must have exactly the opposite convention (optional arguments coming last); we have to check.

This is my break-lunch work.

See you soon.

@victorsndvg
Copy link
Collaborator

@szaghi

an OOP approach will be great!

Modularization hierarchy

I'm completely agree with you.

Modules naming

About modules naming, as a user of your library since some time ago, i think that the actual name functions are friendly. I think that maintaining this names will help the old users to adapt and understand the new modularization.

Maybe something like:

  • VTK_GEO_XML.f90 will contain VTK_GEO_XML_MOD
  • etc.

New branch generation

It's Ok for me. I will follow the changes too near to understand your refactoring improvements

See you.

@szaghi
Copy link
Owner Author

szaghi commented Jun 11, 2015

@victorsndvg

Just added you as collaborators. You can push your commit directly here.

See you.

@victorsndvg
Copy link
Collaborator

@szaghi

About the two previous point talking about auxiliary procedures:

  • I agree with you about passing dummy arguments with name association. It is not only more safety, it is also more elegant :)
  • I thought the opposite you are telling. If I understood correctly the issue, and after searching in the F2003 standard book, I think that the optional arguments are not involved in the uniqueness of a procedure signature. A little piece of the book:

12.6.1 Argument Correspondence
...
Keyword actual argument specification can aid readability by decreasing the need
to remember the precise sequence of dummy arguments in dummy argument lists. The
order-independence also facilitates more flexible use of optional arguments by not re-
quiring omitted arguments to be at the end of the list
. These functionalities, and the
forms that they take, are modeled after keyword specifiers in input/output statements.
...
A procedure reference may use a mixture of positional and keyword correspon-
dence, specifying keywords for some actual arguments but not for others. In this case,
all the actual arguments without keywords must appear prior to those with keywords
in the actual argument list
.
...

It seems that, talking about ambiguous signatures, the order of those mixed arguments affect only to the procedure calling if you don't use the keyword.

I'm not sure if I answered your question, I did?

See you

@szaghi
Copy link
Owner Author

szaghi commented Jun 11, 2015

@victorsndvg

You are right. I was thinking exactly to the sentences you have reported (I had not the standard under my eyes today). I use named associations always alsi for avoid ambiguity. However, the sentence you reported is not talking about the definition of the procedure (as I was concerning) rather to the caller signature. Tomorrow I will check, but lrt us now consider an example:

call foo(a=1,b=2)
call foo(1,b=2)
call foo(1,2) 
call foo(1)

The eventual ambiguity comes from:

  • the caller signature: as you cited all the actual arguments without keywords must appear prior to those with keywords in the actual argument list;
  • but also the order of the definition is important in the case mixed positional and named association is used.

As a consequence, the following definition are not identical:

subroutine foo(a,b)
integer, optional:: a
integer::b
subroutine foo(b,a)
integer::b
integer, optional:: a

The problem is that I don remember which of the above two is better.

Anyhow, tomorrow should be easy to check.

Night!

@victorsndvg
Copy link
Collaborator

@szaghi

It's hard to read all references to optional arguments in the handbook to get what we want to know. Now I'm going to continue reading.

I think that is strongly coherent to put the non optional arguments first, because when you call the procedures without keywords: ...arguments without keywords must appear prior... . I always use it in this way because I think that it's usual in Python.

I hope to get it more clear today

@szaghi
Copy link
Owner Author

szaghi commented Jun 12, 2015

@victorsndvg

My today tests have been usefulness. I cannot discover why (if I remember right) some Intel's Fortran Guru showed me that it was a best practice define optional arguments before non optional....

On the contrary, re-reading a great book, Modern Fortran, style and usage of Clerman and Spector, I found our searching best practice hint

Consistently place subprogram arguments in the following order: the pass argument, intent(in out) arguments, intent(in) arguments, intent (out) arguments, optional arguments.

If you agree, I will follow this convention.

@victorsndvg
Copy link
Collaborator

@szaghi

sorry I cannot found any useful info ...

I'm agree with you. I will take note of this convention also for my future works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants
@szaghi @victorsndvg and others