Skip to content

Comments

Millet new#106

Open
adamavip wants to merge 13 commits intoopenalea:mainfrom
adamavip:millet_new
Open

Millet new#106
adamavip wants to merge 13 commits intoopenalea:mainfrom
adamavip:millet_new

Conversation

@adamavip
Copy link

  • I added a new implementation to build the millet root system as an MTG to architecture module
  • I renamed module turtle to turtle_millet and added visitor2 function to visualize the different root types (crown root are the thicker)
  • Add a notebook to simulate architectures for the lines 57 and 109

NDOUR added 2 commits February 16, 2026 11:38
@pradal pradal requested review from baugetfa and pradal February 16, 2026 13:35
Copy link
Collaborator

@baugetfa baugetfa left a comment

Choose a reason for hiding this comment

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

  • The notebook gives errors see my comments
  • I would change names with only '2' at the end because it is not very descriptive. But it is my point of view.
  • use conductance from hydroot not hydroroot.millet see comments in the ipynb and below.

Copy link
Collaborator

@baugetfa baugetfa Feb 20, 2026

Choose a reason for hiding this comment

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

  1. at the cell Load diameter data for the different root types got the error
TypeError: readCSVFile() got an unexpected keyword argument 'delimiter'

Indeed in readCSVFile their is no delimiter. Just add it like that may be:

def readCSVFile(filename,delimiter=';'):
    """Read and extract data from a csv file, supposed that the data is stored in 2 columns.

    :param filename: string
    :returns: - data (array) - record array of (x, y) values, column headers recorded in dtype

    """
    data = np.genfromtxt(filename,delimiter=delimiter,names=True)
    return data
  1. I do not understand the figure with age. According to the heatmap the tips would be the oldest but it is not the case the tips are the youngest part of the root. So I think I misunderstand something: either it is the time of appearance so it is not the age or the heatmap is inversed.

from openalea.hydroroot.millet import conductance as conductance_millet; reload(conductance)

got

NameError: name 'conductance' is not defined

Remove ; reload(conductance)

  1. Compute K for the lines missing s in view()
s = mtg_scene(g109, prop_cmap='K')
view()
  1. compute radial conducances
    use hydroroot.conductance.fit_property_from_spline instead of hydroroot.millet.conductance it is buggy.
from openalea.hydroroot import conductance
g57 = conductance.fit_property_from_spline(g57, radial_law57, 'position', 'k0')
g57 = conductance.compute_k(g57, k0='k0')
g109 = conductance.fit_property_from_spline(g109, radial_law109, 'position', 'k0')
g109 = conductance.compute_k(g109, k0='k0')

Copy link
Author

Choose a reason for hiding this comment

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

  1. TypeError: readCSVFile() got an unexpected keyword argument 'delimiter'
    I will use pandas.read_csv directly then destructure x and y

  2. Age calculation:
    I think the problem comes from the fact that we fixing the passed to leaf vertices: if g.is_leaf(v):
    age[v] = sr_age
    @pradal can we for every axis, the maximum subtracted from the age of each vertex, so that tips become the youngest

  3. from openalea.hydroroot.millet import conductance as conductance_millet; reload(conductance)
    This is a lapsus should be reload(conductance_millet_

  4. Lapus when using view, should be view(s)

from openalea.mtg.mtg import fat_mtg


def millet_mtg2(
Copy link
Collaborator

Choose a reason for hiding this comment

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

change name.

Copy link
Author

Choose a reason for hiding this comment

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

added 2 as a new implementation comparing the old one but can use another name

Copy link
Contributor

Choose a reason for hiding this comment

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

Proposal:
millet_mtg2 -> generate_millet or millet


#spline = UnivariateSpline(x, y, s=s)
keys = g.property(prop_in).keys()
x_values = np.array(g.property(prop_in).values())
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is buggy. It gives a dict should be x_values = np.array(list(g.property(prop_in).values())) but use instead hydroroot.conductance.fit_property_from_spline and delete this duplicate

Copy link
Contributor

Choose a reason for hiding this comment

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

No. values() returns an iterable.
But I agree that we have to reuse conductance from hydroroot.

Copy link
Contributor

Choose a reason for hiding this comment

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

@baugetfa You are right!
@adamavip Delete this function and use hydroroot.conductance one

law = f
return law

def diameter_law2(data_xy, function=None, plot= True):
Copy link
Collaborator

Choose a reason for hiding this comment

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

change name. '2' is not descriptive

Copy link
Author

Choose a reason for hiding this comment

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

'2' is used just to design a second implementation but agreed not very descriptive

Copy link
Contributor

Choose a reason for hiding this comment

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

Make the opposite if you think you need two functions.
Otherelse, just keep one...

# ************** Here we define evolution diameter laws of different root types************************


def compute_diameter_from_laws(g,
Copy link
Collaborator

Choose a reason for hiding this comment

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

cannot we have either diameter or radius not both as MTG properties. This is the same information and it avoid to have a function like compute_radius_from_diameter ?

Copy link
Author

Choose a reason for hiding this comment

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

the problem is we estimated the diameter from the evolution laws, then consequently calculated the radius afterwards

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but diameter and radius is linked. So just compute radius directly (just /2).

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should have a notebook in src I think

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes shouldn't


def compute_k(g, k0 = 300.):
"""Compute the radial conductance k (:math:`10^{-9}\ m.s^{-1}.MPa^{-1}`) of each vertex of the MTG.
r"""Compute the radial conductance k (:math:`10^{-9}\ m.s^{-1}.MPa^{-1}`) of each vertex of the MTG.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Their is an error here that I corrected it is in fact m^4.s^{-1}.MPa^{-1}
The 'r' is not neccessary their is not bug in the doc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I add the rstring due to a warning for escape charater...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok

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.

3 participants